-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
Allow for multiple --loader flags #18914
Changes from all commits
6e9e023
0d5badf
64fa4d8
e88059f
b03d27d
3bd6e7c
e2a89b9
4f974f0
4906860
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 |
---|---|---|
|
@@ -106,26 +106,45 @@ fs.readFile('./foo.txt', (err, body) => { | |
<!-- type=misc --> | ||
|
||
To customize the default module resolution, loader hooks can optionally be | ||
provided via a `--loader ./loader-name.mjs` argument to Node. | ||
provided via a `--loader ./loader-name.mjs` argument to Node.js. This argument | ||
can be passed multiple times to compose loaders like | ||
`--loader ./loader-coverage.mjs --loader ./loader-mocking.mjs`. The last loader | ||
must explicitly call to the parent loader in order to provide compose behavior. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👆 aren't all loaders used for module loading? Is there a reason to call out the last one as special. Does that mean order is significant? If it's just about calling the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Order is significant. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe just remove "will be used for module loading and"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain a bit more on the significance of the last loader (far right)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From a chaining perspective, the last loader is responsible for maintaining the chain. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't any loader not calling their parent effect the chain? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If a loader doesn't call their parent, then all preceding loaders get ignored. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @devsnek i've removed that phrase. |
||
When hooks are used they only apply to ES module loading and not to any | ||
CommonJS modules loaded. | ||
|
||
All loaders are created by invoking the default export of their module as a | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For ESM, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CJS has the well defined interop steps, so yes |
||
function. The parameters given to the function are a single object with | ||
properties to call the `resolve` and `dynamicInstantiate` hooks of the parent | ||
loader. The default loader has a `resolve` hook and a function that throws for | ||
the value of `dynamicInstantiate`. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👆 By default loader do you mean the last loader (far right)? Or something internal? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. internal. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. he's referring to the default internal loader that is used if you don't hook anything There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the default dynamic instantiate be a function that just throws "no instantiate hook"? Then each caller can assume chaining, with the fallthrough being an error naturally. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it doesn't impact the user exposed code and not something the user can observe (is that right?) would that make more sense as a side note. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. to the user its just a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Specifically, the user should be able to chain the first loader identically to any other loader, so they should specifically call the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've changed the |
||
### Resolve hook | ||
|
||
The resolve hook returns the resolved file URL and module format for a | ||
given module specifier and parent file URL: | ||
|
||
```js | ||
// example loader that treats all files within the current working directory as | ||
// ECMAScript Modules | ||
const baseURL = new URL('file://'); | ||
baseURL.pathname = `${process.cwd()}/`; | ||
|
||
export async function resolve(specifier, | ||
parentModuleURL = baseURL, | ||
defaultResolver) { | ||
export default function(parent) { | ||
return { | ||
url: new URL(specifier, parentModuleURL).href, | ||
format: 'esm' | ||
async resolve(specifier, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why Alternatively, should this just return a function directly? Why the wrapping object? (also, if it's not always meant to be an async function, why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. its called resolve because it resolves the location of a module request. async so you can do whatever magic you need to do e.g. reading files (package.json for instance) without blocking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok - so it doesn't have to be an async function but its return value will always be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually that brings up a good point. in the loader we do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well it is always async, you do need to await it if you aren't just passing it through (ala |
||
parentModuleURL = baseURL) { | ||
const location = new URL(specifier, parentModuleURL); | ||
if (locations.host === baseURL.host && | ||
location.pathname.startsWith(baseURL.pathname)) { | ||
return { | ||
url: location.href, | ||
format: 'esm' | ||
}; | ||
} | ||
return parent.resolve(specifier, parentModuleURL); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps make the example demonstrate chaining, by only applying to packages within the baseURL, while packages outside of the baseURL get the parent resolve or similar? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
}; | ||
} | ||
``` | ||
|
@@ -164,28 +183,35 @@ const JS_EXTENSIONS = new Set(['.js', '.mjs']); | |
const baseURL = new URL('file://'); | ||
baseURL.pathname = `${process.cwd()}/`; | ||
|
||
export function resolve(specifier, parentModuleURL = baseURL, defaultResolve) { | ||
if (builtins.includes(specifier)) { | ||
return { | ||
url: specifier, | ||
format: 'builtin' | ||
}; | ||
} | ||
if (/^\.{0,2}[/]/.test(specifier) !== true && !specifier.startsWith('file:')) { | ||
// For node_modules support: | ||
// return defaultResolve(specifier, parentModuleURL); | ||
throw new Error( | ||
`imports must begin with '/', './', or '../'; '${specifier}' does not`); | ||
} | ||
const resolved = new URL(specifier, parentModuleURL); | ||
const ext = path.extname(resolved.pathname); | ||
if (!JS_EXTENSIONS.has(ext)) { | ||
throw new Error( | ||
`Cannot load file with non-JavaScript file extension ${ext}.`); | ||
} | ||
export default function(parent) { | ||
return { | ||
url: resolved.href, | ||
format: 'esm' | ||
resolve(specifier, parentModuleURL = baseURL) { | ||
if (builtins.includes(specifier)) { | ||
return { | ||
url: specifier, | ||
format: 'builtin' | ||
}; | ||
} | ||
if (/^\.{0,2}[/]/.test(specifier) !== true && | ||
!specifier.startsWith('file:')) { | ||
// For node_modules support: | ||
// return parent.resolve(specifier, parentModuleURL); | ||
throw new Error( | ||
`imports must begin with '/', './', or '../'; '${ | ||
specifier | ||
}' does not`); | ||
} | ||
const resolved = new URL(specifier, parentModuleURL); | ||
const ext = path.extname(resolved.pathname); | ||
if (!JS_EXTENSIONS.has(ext)) { | ||
throw new Error( | ||
`Cannot load file with non-JavaScript file extension ${ext}.`); | ||
} | ||
return { | ||
url: resolved.href, | ||
format: 'esm' | ||
}; | ||
} | ||
}; | ||
} | ||
``` | ||
|
@@ -207,12 +233,25 @@ This hook is called only for modules that return `format: "dynamic"` from | |
the `resolve` hook. | ||
|
||
```js | ||
export async function dynamicInstantiate(url) { | ||
// example loader that can generate modules for .txt files | ||
// that resolved to a 'dynamic' format | ||
import fs from 'fs'; | ||
import util from 'util'; | ||
export default function(parent) { | ||
return { | ||
exports: ['customExportName'], | ||
execute: (exports) => { | ||
// get and set functions provided for pre-allocated export names | ||
exports.customExportName.set('value'); | ||
async dynamicInstantiate(url) { | ||
const location = new URL(url); | ||
if (location.pathname.slice(-4) === '.txt') { | ||
const text = String(await util.promisify(fs.readFile)(location)); | ||
return { | ||
exports: ['text'], | ||
execute: (exports) => { | ||
// get and set functions provided for pre-allocated export names | ||
exports.text.set(text); | ||
} | ||
}; | ||
} | ||
return parent.dynamicInstantiate(url); | ||
} | ||
}; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,14 +2,28 @@ | |
|
||
const { | ||
setImportModuleDynamicallyCallback, | ||
setInitializeImportMetaObjectCallback | ||
setInitializeImportMetaObjectCallback, | ||
} = internalBinding('module_wrap'); | ||
|
||
const getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor; | ||
const hasOwnProperty = Function.call.bind(Object.prototype.hasOwnProperty); | ||
const apply = Reflect.apply; | ||
|
||
const errors = require('internal/errors'); | ||
const { getURLFromFilePath } = require('internal/url'); | ||
const Loader = require('internal/loader/Loader'); | ||
const path = require('path'); | ||
const { URL } = require('url'); | ||
|
||
// fires a getter or reads the value off a descriptor | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What’s the need to support getters? node could mandate that these things all be data properties. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ljharb is there precedent of something that fails when passed a getter but not a data property? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we at least use Reflect.get instead of this whole function There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that i know of in the language. node be as restrictive here as it wants; it could even reject loaders whose resolve functions don’t have the right .length There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @devsnek nope, because that would separate the check for property existing and the reading in a way that proxies could see. They could change their keys between that, generate a new property descriptor between that, etc. This function is also a bit gross because property descriptors inherit from Object.prototype.value = 123;
Object.getOwnPropertyDescriptor({get x(){}}, 'x').value There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not unreliable :-) defaults aren't part of the length. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confusing to the point of not being reliable. Well defined, just not reliable to my eyes. [((g = 0, h) => g)].map(f => f.length)
// 0 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That’s invalid iirc; defaults can’t come before non-defaults. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. huh, TIL. Fair enough. |
||
function grabPropertyOffDescriptor(object, descriptor) { | ||
if (hasOwnProperty(descriptor, 'value')) { | ||
return descriptor.value; | ||
} else { | ||
return apply(descriptor.get, object, []); | ||
} | ||
} | ||
|
||
function normalizeReferrerURL(referrer) { | ||
if (typeof referrer === 'string' && path.isAbsolute(referrer)) { | ||
return getURLFromFilePath(referrer).href; | ||
|
@@ -23,25 +37,81 @@ function initializeImportMetaObject(wrap, meta) { | |
|
||
let loaderResolve; | ||
exports.loaderPromise = new Promise((resolve, reject) => { | ||
loaderResolve = resolve; | ||
loaderResolve = (v) => { | ||
resolve(v); | ||
}; | ||
}); | ||
|
||
exports.ESMLoader = undefined; | ||
|
||
exports.setup = function() { | ||
setInitializeImportMetaObjectCallback(initializeImportMetaObject); | ||
|
||
let ESMLoader = new Loader(); | ||
const RuntimeLoader = new Loader(); | ||
const loaderPromise = (async () => { | ||
const userLoader = process.binding('config').userLoader; | ||
if (userLoader) { | ||
const hooks = await ESMLoader.import( | ||
userLoader, getURLFromFilePath(`${process.cwd()}/`).href); | ||
ESMLoader = new Loader(); | ||
ESMLoader.hook(hooks); | ||
exports.ESMLoader = ESMLoader; | ||
const { userLoaders } = process.binding('config'); | ||
if (userLoaders) { | ||
const BootstrapLoader = new Loader(); | ||
exports.ESMLoader = BootstrapLoader; | ||
let resolve = (url, referrer) => { | ||
return require('internal/loader/DefaultResolve')(url, referrer); | ||
}; | ||
let dynamicInstantiate = (url) => { | ||
throw new errors.Error('ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK'); | ||
}; | ||
for (var i = 0; i < userLoaders.length; i++) { | ||
const loaderSpecifier = userLoaders[i]; | ||
const { default: factory } = await BootstrapLoader.import( | ||
loaderSpecifier); | ||
const cachedResolve = resolve; | ||
const cachedDynamicInstantiate = dynamicInstantiate; | ||
const next = factory({ | ||
__proto__: null, | ||
resolve: Object.setPrototypeOf(async (url, referrer) => { | ||
const ret = await cachedResolve(url, referrer); | ||
return { | ||
__proto__: null, | ||
url: `${ret.url}`, | ||
format: `${ret.format}`, | ||
}; | ||
}, null), | ||
dynamicInstantiate: Object.setPrototypeOf(async (url) => { | ||
const ret = await cachedDynamicInstantiate(url); | ||
return { | ||
__proto__: null, | ||
exports: ret.exports, | ||
execute: ret.execute, | ||
}; | ||
}, null), | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add some comments about these two functions? For somebody not familiar with loaders, they will be hard to read. |
||
const resolveDesc = getOwnPropertyDescriptor(next, 'resolve'); | ||
if (resolveDesc !== undefined) { | ||
resolve = grabPropertyOffDescriptor(next, resolveDesc); | ||
if (typeof resolve !== 'function') { | ||
throw new errors.TypeError('ERR_LOADER_HOOK_BAD_TYPE', | ||
'resolve', 'function'); | ||
} | ||
} | ||
const dynamicInstantiateDesc = getOwnPropertyDescriptor( | ||
next, | ||
'dynamicInstantiate'); | ||
if (dynamicInstantiateDesc !== undefined) { | ||
dynamicInstantiate = grabPropertyOffDescriptor( | ||
next, | ||
dynamicInstantiateDesc); | ||
if (typeof dynamicInstantiate !== 'function') { | ||
throw new errors.TypeError('ERR_LOADER_HOOK_BAD_TYPE', | ||
'dynamicInstantiate', 'function'); | ||
} | ||
} | ||
} | ||
RuntimeLoader.hook({ | ||
resolve, | ||
dynamicInstantiate | ||
}); | ||
} | ||
return ESMLoader; | ||
exports.ESMLoader = RuntimeLoader; | ||
return RuntimeLoader; | ||
})(); | ||
loaderResolve(loaderPromise); | ||
|
||
|
@@ -50,5 +120,5 @@ exports.setup = function() { | |
return loader.import(specifier, normalizeReferrerURL(referrer)); | ||
}); | ||
|
||
exports.ESMLoader = ESMLoader; | ||
exports.RuntimeLoader = RuntimeLoader; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
/* eslint-disable node-core/required-modules */ | ||
export default import.meta.url; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
/* eslint-disable node-core/required-modules */ | ||
import url from './esm-export-url.mjs?x=y'; | ||
process.stdout.write(url); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
/* eslint-disable node-core/required-modules */ | ||
import url from 'url'; | ||
|
||
export default ({ | ||
resolve: parentResolve | ||
}) => ({ | ||
async resolve(specifier, parentModuleURL) { | ||
const parentResolved = await parentResolve(specifier, parentModuleURL); | ||
const request = new url.URL(parentResolved.url); | ||
request.hash = '#hash'; | ||
return { | ||
url: request.href, | ||
format: parentResolved.format | ||
}; | ||
} | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note we do have a folder called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought about using that but we also had other non-test esm files in this folder, can move if desired. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally we could refactor towards There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 That can be done later in another PR |
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.
Does this mean that if any loader forgets to call to the previous loader, it will break the chain?
If so, that seems like a pit of failure; could we make this a pit of success by forcing loaders that want to break the chain to explicitly do so?
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.
perhaps if a hook returns
null
we automatically walk up the chain?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.
I actually like the current design - it pretty much mimics the behavior of middleware in
koa
, which is kinda similar.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.
@devsnek
== null
, probably :-)@weswigham the difference I see here is that if a middleware breaks the contract, your entire request likely fails, because nothing further proceeds; however, depending on what the loaders do, node might silently do the wrong thing.
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 should definitely not just check for undefined. We should require full responsibility to handle all cases and avoid polymorphic return types. We could add something more complicated to model having defaults, but I'm more curious about a code example of this idea of the current design being a pit of failure.
Since the parameters are not sufficient to produce a valid return value, loaders must do something to return a proper value. Is there a real world use case where you are not breaking the chain but are also not fundamentally always calling the parent to do useful things?
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.
fwiw I agree that all of the use cases where "being able to explicitly call the parent loader" are valuable, including "not calling the parent loader" - however, I think it's important that the default, when no explicit choice is made, should be "call the parent loader".
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.
@ljharb the isn't really a way to explicitly say you are not choosing your return value. We can only make well known return values, which I would want to be the same type as the other return values.
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.
@ljharb to clarify. I am not really seeing the case where you want to default to the parent, but do not want to take any action using the result of the parent as the common case. Most loaders want to redirect behaviors or mutate what would be loaded. The only way to know what would be loaded is to query the parent.
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.
I'm not sure that's correct - If a given loader wasn't made with a loader pipeline in mind (ie, never explicitly calls the parent), how am I even sure the loader is capable of yielding output another loader can usefully use?
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.
Like
return null
is also probably more cryptic thanreturn parentResolve()
.