-
-
Notifications
You must be signed in to change notification settings - Fork 33.8k
module: change default resolver to not throw on unknown scheme #47824
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 7 commits
52d276e
94afcae
c87c78c
3df4003
f615248
e4b4cdf
a387b9c
88a4ce9
b0eb1c0
98924dd
357e8db
2c7e695
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 |
|---|---|---|
|
|
@@ -1029,28 +1029,6 @@ and there is no security. | |
| // https-loader.mjs | ||
| import { get } from 'node:https'; | ||
|
|
||
| export function resolve(specifier, context, nextResolve) { | ||
| const { parentURL = null } = context; | ||
|
|
||
| // Normally Node.js would error on specifiers starting with 'https://', so | ||
| // this hook intercepts them and converts them into absolute URLs to be | ||
| // passed along to the later hooks below. | ||
| if (specifier.startsWith('https://')) { | ||
| return { | ||
| shortCircuit: true, | ||
| url: specifier, | ||
| }; | ||
| } else if (parentURL && parentURL.startsWith('https://')) { | ||
| return { | ||
| shortCircuit: true, | ||
| url: new URL(specifier, parentURL).href, | ||
| }; | ||
| } | ||
|
|
||
| // Let Node.js handle all other specifiers. | ||
| return nextResolve(specifier); | ||
| } | ||
|
|
||
| export function load(url, context, nextLoad) { | ||
| // For JavaScript to be loaded over the network, we need to fetch and | ||
| // return it. | ||
|
|
@@ -1091,9 +1069,7 @@ prints the current version of CoffeeScript per the module at the URL in | |
| #### Transpiler loader | ||
|
|
||
| Sources that are in formats Node.js doesn't understand can be converted into | ||
| JavaScript using the [`load` hook][load hook]. Before that hook gets called, | ||
| however, a [`resolve` hook][resolve hook] needs to tell Node.js not to | ||
| throw an error on unknown file types. | ||
| JavaScript using the [`load` hook][load hook]. | ||
|
|
||
| This is less performant than transpiling source files before running | ||
| Node.js; a transpiler loader should only be used for development and testing | ||
|
|
@@ -1109,25 +1085,6 @@ import CoffeeScript from 'coffeescript'; | |
|
|
||
| const baseURL = pathToFileURL(`${cwd()}/`).href; | ||
|
|
||
| // CoffeeScript files end in .coffee, .litcoffee, or .coffee.md. | ||
| const extensionsRegex = /\.coffee$|\.litcoffee$|\.coffee\.md$/; | ||
|
|
||
| export function resolve(specifier, context, nextResolve) { | ||
| if (extensionsRegex.test(specifier)) { | ||
| const { parentURL = baseURL } = context; | ||
|
|
||
| // Node.js normally errors on unknown file extensions, so return a URL for | ||
| // specifiers ending in the CoffeeScript file extensions. | ||
| return { | ||
| shortCircuit: true, | ||
| url: new URL(specifier, parentURL).href, | ||
| }; | ||
| } | ||
|
|
||
| // Let Node.js handle all other specifiers. | ||
| return nextResolve(specifier); | ||
| } | ||
|
|
||
| export async function load(url, context, nextLoad) { | ||
| if (extensionsRegex.test(url)) { | ||
| // Now that we patched resolve to let CoffeeScript URLs through, we need to | ||
|
|
@@ -1220,6 +1177,53 @@ loaded from disk but before Node.js executes it; and so on for any `.coffee`, | |
| `.litcoffee` or `.coffee.md` files referenced via `import` statements of any | ||
| loaded file. | ||
|
|
||
| #### "import map" loader | ||
|
|
||
| The previous two loaders defined `load` hooks. This is an example of a loader | ||
| that does its work via the `resolve` hook. This loader reads an | ||
| `import-map.json` file that specifies which specifiers to override to another | ||
| URL (this is a very simplistic implemenation of a small subset of the | ||
| "import maps" specification). | ||
|
|
||
| ```js | ||
| // import-map-loader.js | ||
| import fs from 'node:fs/promises'; | ||
|
|
||
| const { imports } = JSON.parse(await fs.readFile('import-map.json')); | ||
|
|
||
| export async function resolve(specifier, context, nextResolve) { | ||
| if (specifier in imports) { | ||
giltayar marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| return nextResolve(imports[specifier], context); | ||
| } | ||
|
|
||
| return nextResolve(specifier, context); | ||
| } | ||
| ``` | ||
|
|
||
| Let's assume we have these files: | ||
|
|
||
| ```js | ||
| // main.js | ||
| import 'a-module'; | ||
| ``` | ||
|
|
||
| ```json | ||
| // import-map.json | ||
| { | ||
| "imports": { | ||
| "a-module": "./some-module.js" | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ```js | ||
| // some-module.js | ||
| console.log('some module!'); | ||
| ``` | ||
|
|
||
| If you run `node --experimental-loader ./import-map-loader.js main.js` | ||
| the output will be `some module!`. | ||
|
|
||
| ## Resolution algorithm | ||
|
|
||
| ### Features | ||
|
|
@@ -1506,9 +1510,9 @@ _isImports_, _conditions_) | |
| > 7. If _pjson?.type_ exists and is _"module"_, then | ||
| > 1. If _url_ ends in _".js"_, then | ||
| > 1. Return _"module"_. | ||
| > 2. Throw an _Unsupported File Extension_ error. | ||
| > 2. return **undefined**. | ||
guybedford marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| > 8. Otherwise, | ||
| > 1. Throw an _Unsupported File Extension_ error. | ||
| > 1. return **undefined**. | ||
|
||
|
|
||
| **LOOKUP\_PACKAGE\_SCOPE**(_url_) | ||
|
|
||
|
|
@@ -1581,7 +1585,6 @@ for ESM specifiers is [commonjs-extension-resolution-loader][]. | |
| [custom https loader]: #https-loader | ||
| [load hook]: #loadurl-context-nextload | ||
| [percent-encoded]: url.md#percent-encoding-in-urls | ||
| [resolve hook]: #resolvespecifier-context-nextresolve | ||
| [special scheme]: https://url.spec.whatwg.org/#special-scheme | ||
| [status code]: process.md#exit-codes | ||
| [the official standard format]: https://tc39.github.io/ecma262/#sec-modules | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| import { spawnPromisified } from '../common/index.mjs'; | ||
| import * as fixtures from '../common/fixtures.mjs'; | ||
| import assert from 'node:assert'; | ||
| import { execPath } from 'node:process'; | ||
| import { describe, it } from 'node:test'; | ||
|
|
||
| describe('default resolver', () => { | ||
| // In these tests `byop` is an acronym for "bring your own protocol", and is the | ||
| // protocol our byop-dummy-loader.mjs can load | ||
| it('should accept foreign schemas without exception (e.g. byop://something/or-other)', async () => { | ||
| const { code, stdout, stderr } = await spawnPromisified(execPath, [ | ||
| '--no-warnings', | ||
| '--experimental-loader', | ||
| fixtures.fileURL('/es-module-loaders/byop-dummy-loader.mjs'), | ||
| '--input-type=module', | ||
| '--eval', | ||
| "import 'byop://1/index.mjs'", | ||
| ]); | ||
| assert.strictEqual(code, 0); | ||
| assert.strictEqual(stdout.trim(), 'index.mjs!'); | ||
| assert.strictEqual(stderr, ''); | ||
| }); | ||
|
|
||
| it('should resolve foreign schemas by doing regular url absolutization', async () => { | ||
| const { code, stdout, stderr } = await spawnPromisified(execPath, [ | ||
| '--no-warnings', | ||
| '--experimental-loader', | ||
| fixtures.fileURL('/es-module-loaders/byop-dummy-loader.mjs'), | ||
| '--input-type=module', | ||
| '--eval', | ||
| "import 'byop://1/index2.mjs'", | ||
| ]); | ||
| assert.strictEqual(code, 0); | ||
| assert.strictEqual(stdout.trim(), '42'); | ||
| assert.strictEqual(stderr, ''); | ||
| }); | ||
|
|
||
| // In this test, `byoe` is an acronym for "bring your own extension" | ||
| it('should accept foreign extensions without exception (e.g. ..//something.byoe)', async () => { | ||
| const { code, stdout, stderr } = await spawnPromisified(execPath, [ | ||
| '--no-warnings', | ||
| '--experimental-loader', | ||
| fixtures.fileURL('/es-module-loaders/byop-dummy-loader.mjs'), | ||
| '--input-type=module', | ||
| '--eval', | ||
| "import 'byop://1/index.byoe'", | ||
| ]); | ||
| assert.strictEqual(code, 0); | ||
| assert.strictEqual(stdout.trim(), 'index.byoe!'); | ||
| assert.strictEqual(stderr, ''); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| export function load(url, context, nextLoad) { | ||
| switch (url) { | ||
| case 'byop://1/index.mjs': | ||
| return { | ||
| source: 'console.log("index.mjs!")', | ||
| format: 'module', | ||
| shortCircuit: true, | ||
| }; | ||
| case 'byop://1/index2.mjs': | ||
| return { | ||
| source: 'import c from "./sub.mjs"; console.log(c);', | ||
| format: 'module', | ||
| shortCircuit: true, | ||
| }; | ||
| case 'byop://1/sub.mjs': | ||
| return { | ||
| source: 'export default 42', | ||
| format: 'module', | ||
| shortCircuit: true, | ||
| }; | ||
| case 'byop://1/index.byoe': | ||
| return { | ||
| source: 'console.log("index.byoe!")', | ||
| format: 'module', | ||
| shortCircuit: true, | ||
| }; | ||
| default: | ||
| return nextLoad(url, context); | ||
| } | ||
| } |
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.
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
import-map.jsonshould be given by the user of the loader, so it should be taken from current working directory and not next to the source code of the loader. So I would prefer leaving it like this and not accepting your suggestion. Having said that, I don't feel strongly about it, as this is demo code, so just say the word and I'll commit the suggestion.(obviously, cwd is naive and there should be an env variable or some such, but this is naive code for demo purpose only)
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.
Why not just make the example the
esm.shone then? That’s much simpler and involves only one file, so the overall example would also be shorter.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.
Can you wrap it in
path.resolveand/or add a comment? Without that, it’s a bit confusing IMO.We would have trouble with Windows compatibility, right?
Uh oh!
There was an error while loading. Please reload this page.
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 example I had in mind was to rewrite all bare specifiers like
lodashtohttps://esm.sh/lodash, and that’s it. Add a comment thatloadwould fail without--experimental-network-imports, or without chaining the example HTTPS loader, and call it a day. It should be like a five-line example I’d think.It’s still a bit contrived as presumably you wouldn’t always want to load the latest version of every dependency, but for an example I think it gets the idea across and then real-world implementations can fill in details.
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.
It seems unwise for node docs to bless an arbitrary site in the ecosystem by including it in examples.