-
Notifications
You must be signed in to change notification settings - Fork 0
Add configurable fetcher for import metadata #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -19,6 +19,8 @@ type ImportMeta = ImportInfo & { | |||||||||||||||
| peerImports: string[]; | ||||||||||||||||
| }; | ||||||||||||||||
|
|
||||||||||||||||
| type Fetcher = (url: string | URL) => Promise<Response>; | ||||||||||||||||
|
|
||||||||||||||||
| const KNOWN_TARGETS = new Set([ | ||||||||||||||||
| "es2015", | ||||||||||||||||
| "es2016", | ||||||||||||||||
|
|
@@ -84,13 +86,13 @@ async function addImportImpl( | |||||||||||||||
| target: string, | ||||||||||||||||
| noSRI: boolean, | ||||||||||||||||
| ): Promise<void> { | ||||||||||||||||
| const markedSpecifier = `${specifierOf(imp)}${SPECIFIER_MARK_SEPARATOR}${imp.version}`; | ||||||||||||||||
| const markedSpecifier = specifierOf(imp) + SPECIFIER_MARK_SEPARATOR + imp.version; | ||||||||||||||||
| if (mark.has(markedSpecifier)) { | ||||||||||||||||
| return; | ||||||||||||||||
| } | ||||||||||||||||
| mark.add(markedSpecifier); | ||||||||||||||||
|
|
||||||||||||||||
| const cdnScopeKey = `${cdnOrigin}/`; | ||||||||||||||||
| const cdnScopeKey = cdnOrigin + "/"; | ||||||||||||||||
| const cdnScopeImports = importMap.scopes?.[cdnScopeKey]; | ||||||||||||||||
|
|
||||||||||||||||
| const imports = indirect ? (targetImports ?? ensureScope(importMap, cdnScopeKey)) : importMap.imports; | ||||||||||||||||
|
|
@@ -126,7 +128,7 @@ async function addImportImpl( | |||||||||||||||
| const depSpecifier = specifierOf(depImport); | ||||||||||||||||
| const existingUrl = importMap.imports[depSpecifier] ?? importMap.scopes?.[cdnScopeKey]?.[depSpecifier]; | ||||||||||||||||
| let scopedTargetImports = targetImports; | ||||||||||||||||
| if (existingUrl?.startsWith(`${cdnOrigin}/`)) { | ||||||||||||||||
| if (existingUrl?.startsWith(cdnOrigin + "/")) { | ||||||||||||||||
| const existingImport = parseEsmPath(existingUrl); | ||||||||||||||||
| const existingVersion = valid(existingImport.version); | ||||||||||||||||
| if (existingVersion && depImport.version === existingImport.version) { | ||||||||||||||||
|
|
@@ -138,11 +140,11 @@ async function addImportImpl( | |||||||||||||||
| } | ||||||||||||||||
| if (isPeer) { | ||||||||||||||||
| console.warn( | ||||||||||||||||
| `incorrect peer dependency(unmeet ${depImport.version}): ${depImport.name}@${existingVersion}`, | ||||||||||||||||
| "incorrect peer dependency(unmeet " + depImport.version + "): " + depImport.name + "@" + existingVersion, | ||||||||||||||||
| ); | ||||||||||||||||
| return; | ||||||||||||||||
| } | ||||||||||||||||
| const scope = `${cdnOrigin}/${esmSpecifierOf(imp)}/`; | ||||||||||||||||
| const scope = cdnOrigin + "/" + esmSpecifierOf(imp) + "/"; | ||||||||||||||||
| scopedTargetImports = ensureScope(importMap, scope); | ||||||||||||||||
| } | ||||||||||||||||
| } | ||||||||||||||||
|
|
@@ -226,11 +228,11 @@ function parseImportSpecifier(specifier: string): ImportInfo { | |||||||||||||||
| [packageAndVersion, imp.subPath] = splitByFirst(source, "/"); | ||||||||||||||||
| [imp.name, imp.version] = splitByFirst(packageAndVersion, "@"); | ||||||||||||||||
| if (scopeName) { | ||||||||||||||||
| imp.name = `${scopeName}/${imp.name}`; | ||||||||||||||||
| imp.name = scopeName + "/" + imp.name; | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| if (!imp.name) { | ||||||||||||||||
| throw new Error(`invalid package name or version: ${specifier}`); | ||||||||||||||||
| throw new Error("invalid package name or version: " + specifier); | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| return imp; | ||||||||||||||||
|
|
@@ -245,20 +247,24 @@ function normalizeTarget(target: string | undefined): string { | |||||||||||||||
|
|
||||||||||||||||
| function getCdnOrigin(cdn: string | undefined): string { | ||||||||||||||||
| if (cdn && (cdn.startsWith("https://") || cdn.startsWith("http://"))) { | ||||||||||||||||
| return cdn.replace(/\/+$/, ""); | ||||||||||||||||
| if (cdn.endsWith("/")) { | ||||||||||||||||
| // remove trailing slash | ||||||||||||||||
| return cdn.slice(0, -1); | ||||||||||||||||
| } | ||||||||||||||||
| return cdn; | ||||||||||||||||
|
||||||||||||||||
| if (cdn.endsWith("/")) { | |
| // remove trailing slash | |
| return cdn.slice(0, -1); | |
| } | |
| return cdn; | |
| // remove one or more trailing slashes to normalize the origin | |
| return cdn.replace(/\/+$/, ""); |
Copilot
AI
Feb 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSDoc @param tag says @param fetcher but the actual parameter is named f. This mismatch means documentation tooling will not correctly associate the parameter description with the actual parameter. The parameter should be renamed from f to fetcher to match the docstring and to be more descriptive.
Copilot
AI
Feb 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error variable in a catch clause is typed as unknown in TypeScript (with useUnknownInCatchVariables, which is on by default in strict mode and in Bun's TypeScript). Accessing error.message directly will cause a TypeScript type error. The previous code avoided this by not capturing the error variable at all (catch {), and instead included bodyText.slice(0, 200) in the error message, which provided useful debugging context. The new code should either cast the error to Error using error instanceof Error ? error.message : String(error), or restore the previous bodyText.slice(0, 200) approach.
| throw new Error("invalid meta response from " + url + ": " + error.message); | |
| const message = error instanceof Error ? error.message : String(error); | |
| throw new Error("invalid meta response from " + url + ": " + message); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In both mocked tests,
setFetcheris called before thetry/finallyblock that restores the fetcher. In the "warns on unmet peer dependency" test,setFetcher(line 78) andspyOn(console, "warn")(line 111) both run before thetryblock (line 112). IfspyOnthrows, the mock fetcher is never restored and will leak into subsequent tests. Similarly, in the "removes scope specifiers..." test,setFetcher(line 138) runs before thetryblock (line 157). ThesetFetchercall should be moved inside thetryblock (or placed at the very top of it), just asspyOnis, so that thefinallycleanup always runs after the setup.