esm: move hooks handling into separate class #45869
Conversation
GeoffreyBooth
added
module
|
Review requested:
|
nodejs-github-bot
added
needs-ci
@aduh95 The Should we be using |
|
I ran some benchmarks. Module ones seem like a wash, or slight improvement: Startup ones are maybe concerning? @joyeecheung? Could this have anything to do with moving some files to lazy-load that perhaps should instead be inside the snapshot? |
My guess is that using a null-prototype object could be surprising for the user, almost everything in JS inherits from
|
GeoffreyBooth
force-pushed
the
esm-loader-hooks-class
branch
from
ca876ad
to
3f7732e
Compare
The top-level scopes LGTM |
Would it be better to move say |
If it's going to be used by, e.g. 50%+ of every user of Node.js, then it's a good idea to move it into the snapshot. However load.js itself is problematic (top-level access to run-time states, and eager-loading a bunch of other modules that also do this, and some funny TDZ and circular dependencies in between), so refactoring of its entire dependency graph would be necessary before it can be snapshotted. |
This comment was marked as outdated.
This comment was marked as outdated.
GeoffreyBooth
force-pushed
the
esm-loader-hooks-class
branch
from
3f7732e
to
2babd50
Compare
|
@GeoffreyBooth I can resolve the merge conflicts tomorrow if you'd like |
|
If you want to wait, yes. |
GeoffreyBooth
removed
author ready
|
@nodejs/releasers This should now land cleanly once #43772 lands first. |
juanarbol
added
the
backport-requested-v18.x
|
This is not landing cleanly for v18.x as well |
It depends on #43772; have you tried landing that first? |
|
Same for v19.x
Yes, #43772 landed first. For reference, here is the resulting patch I get once I try to land it on top of #43772 in git diffdiff --cc lib/internal/modules/esm/loader.js
index a02619818ec,5c4ee53542b..00000000000
--- a/lib/internal/modules/esm/loader.js
+++ b/lib/internal/modules/esm/loader.js
@@@ -6,18 -6,11 +6,16 @@@ require('internal/modules/cjs/loader')
const {
Array,
ArrayIsArray,
- ArrayPrototypeJoin,
- ArrayPrototypePush,
FunctionPrototypeCall,
++<<<<<<< HEAD
+ ObjectAssign,
+ ObjectDefineProperty,
++=======
+ ObjectCreate,
++>>>>>>> 7738844fe35 (esm: move hooks handling into separate class)
ObjectSetPrototypeOf,
- RegExpPrototypeExec,
SafePromiseAllReturnArrayLike,
SafeWeakMap,
- StringPrototypeSlice,
- StringPrototypeToUpperCase,
- globalThis,
} = primordials;
const {
@@@ -244,118 -106,13 +111,77 @@@ class ESMLoader
}
}
++<<<<<<< HEAD
+ /**
+ *
+ * @param {ModuleExports} exports
+ * @returns {ExportedHooks}
+ */
+ static pluckHooks({
+ globalPreload,
+ resolve,
+ load,
+ // obsolete hooks:
+ dynamicInstantiate,
+ getFormat,
+ getGlobalPreloadCode,
+ getSource,
+ transformSource,
+ }) {
+ const obsoleteHooks = [];
+ const acceptedHooks = { __proto__: null };
+
+ if (getGlobalPreloadCode) {
+ globalPreload ??= getGlobalPreloadCode;
+
+ process.emitWarning(
+ 'Loader hook "getGlobalPreloadCode" has been renamed to "globalPreload"'
+ );
+ }
+ if (dynamicInstantiate) ArrayPrototypePush(
+ obsoleteHooks,
+ 'dynamicInstantiate'
+ );
+ if (getFormat) ArrayPrototypePush(
+ obsoleteHooks,
+ 'getFormat',
+ );
+ if (getSource) ArrayPrototypePush(
+ obsoleteHooks,
+ 'getSource',
+ );
+ if (transformSource) ArrayPrototypePush(
+ obsoleteHooks,
+ 'transformSource',
+ );
+
+ if (obsoleteHooks.length) process.emitWarning(
+ `Obsolete loader hook(s) supplied and will be ignored: ${
+ ArrayPrototypeJoin(obsoleteHooks, ', ')
+ }`,
+ 'DeprecationWarning',
+ );
+
+ if (globalPreload) {
+ acceptedHooks.globalPreload = globalPreload;
+ }
+ if (resolve) {
+ acceptedHooks.resolve = resolve;
+ }
+ if (load) {
+ acceptedHooks.load = load;
+ }
+
+ return acceptedHooks;
++=======
+ addCustomLoaders(userLoaders) {
+ const { Hooks } = require('internal/modules/esm/hooks');
+ this.#hooks = new Hooks(userLoaders);
++>>>>>>> 7738844fe35 (esm: move hooks handling into separate class)
}
- /**
- * Collect custom/user-defined hook(s). After all hooks have been collected,
- * the global preload hook(s) must be called.
- * @param {KeyedExports} customLoaders
- * A list of exports from user-defined loaders (as returned by
- * ESMLoader.import()).
- */
- addCustomLoaders(
- customLoaders = [],
- ) {
- for (let i = 0; i < customLoaders.length; i++) {
- const {
- exports,
- url,
- } = customLoaders[i];
- const {
- globalPreload,
- resolve,
- load,
- } = ESMLoader.pluckHooks(exports);
-
- if (globalPreload) {
- ArrayPrototypePush(
- this.#hooks.globalPreload,
- {
- fn: globalPreload,
- url,
- },
- );
- }
- if (resolve) {
- ArrayPrototypePush(
- this.#hooks.resolve,
- {
- fn: resolve,
- url,
- },
- );
- }
- if (load) {
- ArrayPrototypePush(
- this.#hooks.load,
- {
- fn: load,
- url,
- },
- );
- }
- }
+ preload() {
+ this.#hooks?.preload();
}
async eval(
@@@ -771,35 -296,24 +365,24 @@@
* @param {string} originalSpecifier The specified URL path of the module to
* be resolved.
* @param {string} [parentURL] The URL path of the module's parent.
- * @param {ImportAssertions} [importAssertions] Assertions from the import
+ * @param {ImportAssertions} importAssertions Assertions from the import
* statement or expression.
- * @returns {{ format: string, url: URL['href'] }}
+ * @returns {Promise<{ format: string, url: URL['href'] }>}
*/
async resolve(
originalSpecifier,
parentURL,
- importAssertions = ObjectCreate(null),
+ importAssertions,
) {
- const isMain = parentURL === undefined;
-
- if (
- !isMain &&
- typeof parentURL !== 'string' &&
- !isURLInstance(parentURL)
- ) {
- throw new ERR_INVALID_ARG_TYPE(
- 'parentURL',
- ['string', 'URL'],
- parentURL,
- );
+ if (this.#hooks) {
+ return this.#hooks.resolve(originalSpecifier, parentURL, importAssertions);
+ }
+ if (!this.#defaultResolve) {
+ this.#defaultResolve = require('internal/modules/esm/resolve').defaultResolve;
}
- const chain = this.#hooks.resolve;
const context = {
- conditions: getDefaultConditions(),
+ __proto__: null,
+ conditions: this.#defaultConditions,
importAssertions,
parentURL,
}; |
PR-URL: nodejs#45869 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#45869 Backport-PR-URL: nodejs#46511 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#45869 Backport-PR-URL: nodejs#46511 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #45869 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs/node#45869 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs/node#45869 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This PR splits apart the
ESMLoaderclass into two classes,ESMLoaderandHooks, where the latter’s file is only required (and the class instantiated) when--loaderis used to register custom user loaders. As part of this, a few otherlib/internal/module/esmfiles were changed to become lazy-loaded (cc @joyeecheung, please tell me if I’m doing this right).This should result in a minor performance boost for the default case of no user loaders registered, as we don’t need to do as much validation for the return values of Node’s internal
defaultResolveanddefaultLoadhooks; those functions are also now called directly, rather than as part of a “chain” of one each. cc @mcollinaThe case where user loaders are registered will now consume slightly more memory than before. Both previously and in this PR there is a check where the
urlproperty passed between hooks (as a return value fromresolveand an input value forload) is validated that it can be instantiated vianew URL(url)without throwing. Before this PR, there’s a check that theurlwe’re about to validate hasn’t already been validated by virtue of it existing inESMLoader.moduleMap. Per this PR, we cache already-validatedurlvalues in a newSafeSetthat’s attached to theHooksclass. This new data structure means that we store all of the URLs for all loaded modules twice, inESMLoader.moduleMapand inHooks.#validatedUrls; but I think this is unavoidable if we want this to become the basis for #44710 since we wouldn’t be able to shareESMLoader.moduleMapacross the worker boundary. It’s arguable whether we need to do this validation check at all, or at least to do it between every registered hook; if we did the check only once on the final result of theresolvechain, it could stay inESMLoader. This would be a breaking change to the current Loaders API, however, so I’m avoiding making such a change as part of this PR. (I think we should consider if it’s a change worth making, and the tradeoffs of whether the check is worth the performance cost, but I think that can come in a later PR.)This should set the stage for another attempt at wrapping the user loaders processing into a worker thread. In #44710 the idea was to put a second
ESMLoaderclass inside the worker and try to send instantiatedModuleout from the worker back to the main thread, but those proved impossible to serialize. After this PR lands, thehooks.jsthat this PR adds would be what gets wrapped in a worker, and all of its inputs and outputs should be serializable. (They’re not quite JSON serializable, as theloadhook returns an object with asourceproperty that could be a buffer, but these return values are much simpler thanModuleobjects and should be within the capabilities ofv8.serialize/deserialize.)The bulk of
hooks.jswas just moved fromESMLoader. I avoided making unrelated changes other than adding/editing some comments and JSDocs.cc @nodejs/loaders