Allow for multiple --loader flags #18914

bmeck · 2018-02-21T20:19:16Z

This allows multiple --loader flags to be used at a given time. So if someone is doing node --loader ./my-loader.mjs they could add a second --loader for APM or mocking purposes

NODE_OPTIONS='--experimental-modules --loader ./mock-loader.mjs' node --experimental-modules  --loader ./my-loader.mjs
node --experimental-modules --loader ./mock-loader.mjs --loader ./my-loader.mjs

We should probably do some design work around this composition API. I've tried to prevent loaders from passing arbitrary data between each other to guard against tight coupling. There is also a problem of child loaders being able to disagree with parents on the format of a URL rather than having a design that enforces a single value for the format.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

module

targos · 2018-02-21T20:21:07Z

/cc @nodejs/modules

jdalton · 2018-02-21T20:29:06Z

Allowing multiple loaders is good thing.

jdalton · 2018-02-21T20:44:34Z

doc/api/esm.md

+properties to call the `resolve` and `dynamicInstantiate` hooks of the parent
+loader. The default loader has a `resolve` hook and `null` for the value
+`dynamicInstantiate`.
+


👆 By default loader do you mean the last loader (far right)? Or something internal?

he's referring to the default internal loader that is used if you don't hook anything

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.

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.

to the user its just a parent with { resolve: [Function], dynamicInstantiate: null }, they wouldn't (and probably shouldn't) have any knowledge of where in the chain they are

Specifically, the user should be able to chain the first loader identically to any other loader, so they should specifically call the dynamicInstantiate if creating their own to ensure chaining. Thus the first one as throwing is needed to maintain this equivalence (although the loader needn't necessarily need to know that it is throwing).

I've changed the dynamicInstantiate to be a throwing function.

jdalton · 2018-02-21T20:45:59Z

doc/api/esm.md

+can be passed multiple times to compose loaders like
+`--loader ./loader-coverage.mjs --loader ./loader-mocking.mjs`. The last loader
+will be used for module loading and must explicitly call to the parent loader
+in order to provide compose behavior.



👆 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 parent there may be a way to word that better. Something like how acorn plugins allow calling a parent in case there are other plugins.

Order is significant.

maybe just remove "will be used for module loading and"?

Can you explain a bit more on the significance of the last loader (far right)?

From a chaining perspective, the last loader is responsible for maintaining the chain.

Can't any loader not calling their parent effect the chain?

If a loader doesn't call their parent, then all preceding loaders get ignored.

@devsnek i've removed that phrase.

vsemozhetbyt · 2018-02-21T23:14:08Z

doc/api/esm.md

@@ -106,11 +106,20 @@ 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. This argument


A nit: Node -> Node.js

vsemozhetbyt · 2018-02-21T23:15:53Z

doc/api/esm.md


 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
+function. The parameters given to the the function is a single object with


the the -> the

ljharb

"Multiple loaders" is super important 🎉

ljharb · 2018-02-21T23:40:55Z

doc/api/esm.md

+provided via a `--loader ./loader-name.mjs` argument to Node. 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.


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?

perhaps if a hook returns null we automatically walk up the chain?

I actually like the current design - it pretty much mimics the behavior of middleware in koa, which is kinda similar.

@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.

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?

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".

@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.

@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.

however, I think it's important that the default, when no explicit choice is made, should be "call the parent loader".

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?

Like return null is also probably more cryptic than return parentResolve().

ljharb · 2018-02-21T23:41:20Z

doc/api/esm.md


 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


For ESM, default, for CJS, module.exports? (since a loader could be either)

CJS has the well defined interop steps, so yes module.exports for CJS / .node / .json / ...

ljharb · 2018-02-21T23:42:31Z

doc/api/esm.md

  return {
-    url: new URL(specifier, parentModuleURL).href,
-    format: 'esm'
+    async resolve(specifier,


why resolve? If this is always meant to be an async function, should it just be called then, so that Promise.resolve-wrapping it always does what's expected?

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 async here? Is the return value always wrapped in Promise.resolve?

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

ok - so it doesn't have to be an async function but its return value will always be awaited?

actually that brings up a good point. in the loader we do await but people making hooks might not know to use the same abstraction when calling parent hooks

well it is always async, you do need to await it if you aren't just passing it through (ala return parent.resolve(...)).

ljharb · 2018-02-21T23:55:52Z

lib/internal/process/modules.js

+        let next = factory({
+          __proto__: null,
+          resolve: Object.setPrototypeOf(async (url, referrer) => {
+                const ret = await cachedResolve(url, referrer);


is this node core style rules, or is this function body indentation one level too deep?

it was bad, fixed

bnoordhuis · 2018-02-22T08:51:29Z

src/node_config.cc

+      auto loaders = v8::Array::New(isolate, config_userland_loaders.size());
+      for (size_t i = 0; i < config_userland_loaders.size(); i++) {
+        auto item = config_userland_loaders[i];
+        loaders->Set(


Can you use the overload that takes a Local<Context> and call .FromJust() on the return value?

bnoordhuis · 2018-02-22T08:52:06Z

src/node_config.cc

+                              v8::NewStringType::kNormal).ToLocalChecked());
+      }
+
+      loaders->SetIntegrityLevel(context, v8::IntegrityLevel::kFrozen);


.FromJust()

ljharb · 2018-02-22T15:34:54Z

lib/internal/process/modules.js

 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


What’s the need to support getters? node could mandate that these things all be data properties.

@ljharb is there precedent of something that fails when passed a getter but not a data property?

can we at least use Reflect.get instead of this whole function

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

@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 and people can taint value / get and we don't want to use Reflect.get because it would grab those as well:

Object.prototype.value = 123; Object.getOwnPropertyDescriptor({get x(){}}, 'x').value

That's not unreliable :-) defaults aren't part of the length.

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

That’s invalid iirc; defaults can’t come before non-defaults.

Naw, it's fine:

huh, TIL. Fair enough.

bmeck · 2018-02-22T15:54:59Z

Older iterations of this used new to allow the factory to be a class. Does anyone have opinions on changing this back to use new?

ljharb · 2018-02-22T15:56:16Z

I prefer a bare function; those who want a class can (...args) => new x(...args)

devsnek · 2018-02-22T15:57:50Z

will classes have to be used for proper context with per-package hooks? I would like all loaders to be the same format no matter how they are used

bmeck · 2018-02-22T16:00:52Z

@devsnek they would match whatever global hooks are generated using.

guybedford

Just a couple of comments, but otherwise this seems great.

guybedford · 2018-02-23T13:42:52Z

doc/api/esm.md

+        url: new URL(specifier, parentModuleURL).href,
+        format: 'esm'
+      };
+    }


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?

guybedford · 2018-02-23T13:43:09Z

doc/api/esm.md

+          // get and set functions provided for pre-allocated export names
+          exports.customExportName.set('value');
+        }
+      };


Similarly, perhaps this can be filtered to demonstrate chaining as well?

guybedford · 2018-02-23T13:45:07Z

lib/internal/process/modules.js

@@ -31,15 +45,65 @@ exports.ESMLoader = undefined;
 exports.setup = function() {
  setInitializeImportMetaObjectCallback(initializeImportMetaObject);

-  let ESMLoader = new Loader();
+  const ESMLoader = new Loader();


Note that this was a let because a separate loader was used to load the hook loaders than to load the main in order to support that any dependencies of the loaders themselves can support hooking in the main app.

this is a regression, nice catch. should we give each loader its own module map space, or just the loaders vs runtime?

i've split this into 2 distinct loaders. I did notice a few odd things right now outside of this PR:

it looks like import() always uses the bootstrap loader if a custom loader exists :

node/lib/internal/process/modules.js

Lines 34 to 51 in 3a19122

let ESMLoader = 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;

}

return ESMLoader;

})();

loaderResolve(loaderPromise);

setImportModuleDynamicallyCallback(async (referrer, specifier) => {

const loader = await loaderPromise;

return loader.import(specifier, normalizeReferrerURL(referrer));

});

it looks like the bootstrap loader and the runtime loader are sharing require.cache so the isolation doesn't fully hold for CJS stuff.

guybedford · 2018-02-23T13:47:32Z

test/es-module/loader-add-hash.mjs

+      format: parentResolved.format
+    };
+  }
+});


Note we do have a folder called test/fixtures/es-module-loaders for the loaders themselves which may be worth using here for these new loader cases.

I thought about using that but we also had other non-test esm files in this folder, can move if desired.

Ideally we could refactor towards test/es-module just containing the direct top-level test files, with everything else in fixtures directories?

👍 That can be done later in another PR

targos · 2018-02-23T16:15:05Z

doc/api/errors.md

+<a id="ERR_LOADER_HOOK_BAD_TYPE"></a>
+### ERR_LOADER_HOOK_BAD_TYPE
+
+An Loader defined an invalid value for a hook. See the documentation for [ECMAScript Modules][] for more information.


Nit: A loader

Nit: also line wrapping needed

bmeck · 2018-02-27T17:12:28Z

This has been sitting for a while. If there is nothing in the next 24 hours I plan on merging tomorrow.

mcollina

LGTM with a nit

mcollina · 2018-02-28T20:29:51Z

lib/internal/process/modules.js

+              execute: ret.execute,
+            };
+          }, null),
+        });


Can you add some comments about these two functions? For somebody not familiar with loaders, they will be hard to read.

BridgeAR · 2018-03-02T01:47:34Z

This needs a rebase.

BridgeAR · 2018-04-28T13:56:13Z

What is the status here?

devsnek · 2018-04-28T14:14:35Z

@BridgeAR blocked because its a new feature and modules team doesn't want to land new features atm

guybedford · 2018-05-16T21:23:29Z

@bmeck do you think we could get this going again? Seems related to nodejs/modules#82 as well.

bmeck · 2018-05-16T21:27:46Z

@guybedford I've put moratorium on my PRs for anything outside of vm work and bugfixes, if the module wg wants to give a go ahead that seems fine.

guybedford · 2018-05-16T21:29:16Z

@bmeck would it help if I rebase and continue to drive this one forward? Feels important to me.

bmeck · 2018-05-16T21:37:09Z

@guybedford feel free to take it

viktor-ku

PR looks promising but as a loader author I will need to check specifier within the loader instead of calling the loader if and only if it's required for resolving specific import. And I feel like it can save a lot of time to call specific loader for specific import instead of just throwing every import to loaders

UPD Issue closed

TL;DR; it is slow to test loaders by regexp and load only if necessary

viktor-ku

PR looks promising

jasnell · 2018-10-17T17:20:22Z

What's the status on this?

bmeck · 2018-10-17T17:32:17Z

@jasnell Still stuck due to Modules group moratorium.

Trott · 2018-11-13T20:41:28Z

Still stalled on @nodejs/modules?

bmeck · 2018-11-13T20:43:59Z

@Trott yup

BridgeAR · 2019-03-09T23:49:34Z

@bmeck is this something that might be worked on again anytime soon? Otherwise I suggest we close this for now and you reopen it as soon as there is agreement in the modules working group about it?

guybedford · 2019-03-10T12:35:56Z

@BridgeAR this work also exists as part of the new loaders work which @bmeck started, so that effectively takes over from this PR already. Closing sounds sensible to me.

It now refuses to run if 1. User has not configured upstream or branch - no more defaults. 2. User is not on the branch configured 3. User is on detached HEAD Refs: nodejs/node#18914 Fixes: nodejs/node-core-utils#198

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. labels Feb 21, 2018

targos added the esm Issues and PRs related to the ECMAScript Modules implementation. label Feb 21, 2018

devsnek approved these changes Feb 21, 2018

View reviewed changes

jdalton reviewed Feb 21, 2018

View reviewed changes

vsemozhetbyt reviewed Feb 21, 2018

View reviewed changes

ljharb reviewed Feb 22, 2018

View reviewed changes

bnoordhuis reviewed Feb 22, 2018

View reviewed changes

ljharb reviewed Feb 22, 2018

View reviewed changes

benjamingr approved these changes Feb 22, 2018

View reviewed changes

guybedford requested changes Feb 23, 2018

View reviewed changes

guybedford approved these changes Feb 23, 2018

View reviewed changes

targos reviewed Feb 23, 2018

View reviewed changes

bmeck mentioned this pull request Feb 23, 2018

--loader hooks for require() nodejs/modules#6

Closed

jasnell approved these changes Feb 28, 2018

View reviewed changes

mcollina approved these changes Feb 28, 2018

View reviewed changes

bmeck added 5 commits March 5, 2018 08:27

Allow for multiple --loader flags

6e9e023

default dynamicInstantiate to throw and add type check

0d5badf

doc touchup

64fa4d8

type check test

e88059f

doc nits

b03d27d

BridgeAR added the stalled Issues and PRs that are stalled. label Apr 28, 2018

MylesBorins added the blocked PRs that are blocked by other issues or PRs. label Apr 28, 2018

bmeck mentioned this pull request May 16, 2018

introduce loader hook context #20761

Closed

4 tasks

bmeck mentioned this pull request Jun 19, 2018

esm: Make custom loaders specific #21415

Closed

viktor-ku reviewed Jun 20, 2018

View reviewed changes

viktor-ku approved these changes Jun 20, 2018

View reviewed changes

guybedford mentioned this pull request Jun 24, 2018

Implement createObjectURL/Blob from File API #16167

Closed

5 tasks

bmeck mentioned this pull request Jun 26, 2018

Proposal for transparent interop implementation nodejs/modules#137

Closed

devsnek mentioned this pull request Jul 19, 2018

Have presentation on loaders. nodejs/modules#135

Closed

bmeck mentioned this pull request Aug 14, 2018

Proposals for configuring parse goals of files in --experimental-modules nodejs/modules#160

Closed

guybedford closed this Mar 10, 2019

reasonablytall mentioned this pull request Jul 17, 2019

Loader Hooks nodejs/modules#351

Closed

bmeck mentioned this pull request May 1, 2020

Usage of Loader hook as APM/RASP vendors nodejs/modules#508

Closed

	let ESMLoader = 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;
	}
	return ESMLoader;
	})();
	loaderResolve(loaderPromise);

	setImportModuleDynamicallyCallback(async (referrer, specifier) => {
	const loader = await loaderPromise;
	return loader.import(specifier, normalizeReferrerURL(referrer));
	});

Allow for multiple --loader flags #18914

Allow for multiple --loader flags #18914

Conversation

bmeck commented Feb 21, 2018

Checklist

Affected core subsystem(s)

targos commented Feb 21, 2018

jdalton commented Feb 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

jdalton Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

weswigham Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

weswigham Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck commented Feb 22, 2018

ljharb commented Feb 22, 2018

devsnek commented Feb 22, 2018 • edited Loading

bmeck commented Feb 22, 2018

guybedford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Feb 21, 2018 •

edited

Loading

bmeck Feb 21, 2018 •

edited

Loading

jdalton Feb 21, 2018 •

edited

Loading

devsnek Feb 21, 2018 •

edited

Loading

bmeck Feb 22, 2018 •

edited

Loading

weswigham Feb 22, 2018 •

edited

Loading

weswigham Feb 22, 2018 •

edited

Loading

devsnek Feb 22, 2018 •

edited

Loading

ljharb Feb 22, 2018 •

edited

Loading

devsnek commented Feb 22, 2018 •

edited

Loading