-
Notifications
You must be signed in to change notification settings - Fork 81
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
replace importmap with import rewrite #101
Conversation
@@ -105,3 +109,11 @@ function isNamespaceSpecifier(node) { | |||
function isNotNamespaceSpecifier(node) { | |||
return node.type !== "ImportNamespaceSpecifier"; | |||
} | |||
|
|||
export function resolveImport(specifier: string): string { | |||
return !specifier.startsWith("npm:") |
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.
could we define
const CDN_IMPORT_SPECIFIER = "npm:"
I think it makes the code a little easier to read:
return !specifier.startsWith(CDN_IMPORT_SPECIFIER)
? specifier
: specifier === `${CDN_IMPORT_SPECIFIER}@observable/runtime`
? "/_observablehq/runtime.js"
: `https://cdn.jsdelivr.net/npm/${specifier.slice(CDN_IMPORT_SPECIFIER.length)}/+esm`
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.
We could, but I think it would just make it less obvious what the code is doing, since you would have to then lookup what the value of CDN_IMPORT_SPECIFIER is. And this isn’t the sort of thing that would change — it’s a contract with users, who will use the npm: “protocol” to indicate a package that is published to npm (in the same way that Node.js developers use the node: protocol to indicate built-in packages that Node provides as opposed to ones installed into node_modules).
https://2ality.com/2021/12/node-protocol-imports.html
If anything, I would pull out the code that maps an npm module specifier to a URL (currently using jsDelivr’s +esm service), as that is an implementation detail and not something that a user would typically care about. We could make that a constant too, but a function is better because it’s not a fixed prefix on other CDNs (e.g., esm.sh). But that could consolidate a bit if desired.
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.
That would look like this:
export function resolveImport(specifier: string): string {
return !specifier.startsWith("npm:") ? specifier : resolveNpmImport(specifier.slice(4));
}
export function resolveNpmImport(specifier: string): string {
return specifier === "@observablehq/runtime"
? "/_observablehq/runtime.js"
: `https://cdn.jsdelivr.net/npm/${specifier}/+esm`;
}
I’d rather keep things as-is, though, since resolveImport is already nicely encapsulated; I don’t see a need to expose a separate method for resolving a specifier that is specific to npm.
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.
Rewritten as "npm:".length
instead of 4 for clarity.
b45e36f
to
73e57bf
Compare
Fixes #14. Instead of relying on an import map, we now rewrite
npm:foo
import specifiers to the corresponding URL. Sincepublic/client.js
is served statically without rewriting, we replace a handful ofnpm:
import specifiers with the corresponding URL to match.Screen.Recording.2023-11-04.at.7.11.55.AM.mov