loader: refactor loader #16874

devsnek · 2017-11-07T22:14:50Z

The loader was refactored to use some better terms and methods, mostly
inspired by the whatwg loader spec. Namely there is now translators
instead of loaders and registry instead of moduleMap. The loader
is also exposed as a builtin, allowing users to make their own loaders
and hook the main loader.

I also found a test that wanted to check for the values of exports being unchanged, but I think thats not really correct, and only succeeded as an edge case, and since it was failing after the refactor I removed it. If this was erroneous please let me know.

I'm going to avoid documenting the whole loader until this gets some backing.

cc @bmeck @guybedford @addaleax

Checklist

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

Affected core subsystem(s)

loader

devsnek · 2017-11-08T01:28:55Z

one thing i wonder, as ModuleJob#reflect isn't used should it be removed? It basically allows the equivalent of messing with require.cache

bmeck · 2017-11-08T12:43:31Z

lib/internal/loader/Loader.js

 const debug = require('util').debuglog('esm');

+const kDynamicTranslate = Symbol('dynamicTranslate');


Why is this a symbol if it is essentially a publicly mutable property?

I want to force people to use hook, in case we have any internal structure changes in the future, and I should probably do the same with resolve

They can get this via Object.getOwnPropertySymbols, I don't think this is private.

@bmeck i think i'll use a closure but if you have a better idea lemme know. technically the loader spec wants them to be as symbols (which is why i did Loader.resolve) but i'm not too sure about that

bmeck · 2017-11-08T12:44:32Z

lib/internal/loader/Loader.js

-    this.resolver = resolve.bind(null);
-    this.dynamicInstantiate = dynamicInstantiate;
+    this.base = base;
+    this.registry = new Registry();


Please use current standard terminology: https://html.spec.whatwg.org/multipage/webappapis.html#concept-settings-object-module-map

I agree, the code should keep using that terminology. It sucks that that really makes it less comprehensible, though … so I would really not mind any kind of comments to explain what’s happening

bmeck · 2017-11-08T12:49:49Z

Please use the standard terminology from published specs like module map. The author of the loader spec recommends excluding it from implementations for now.

bmeck · 2017-11-08T12:47:11Z

test/es-module/test-esm-snapshot.mjs

-import one from './esm-snapshot';
-import assert from 'assert';
-
-assert.strictEqual(one, 1);


this file is done to test timing of snapshotting. It should not be removed

so this test is checking if mutating require.cache affects the imported modules? Seeing as module._load is used wouldn't this be the expected behavior?

wouldn't this be the expected behavior?

I don't know what "this" is.

sorry about that, i meant wouldn't

importing the mutator

importing the mutatee (i hope thats a word)

mutatee being mutated

be the expected behavior

If we walk thought the code the following happens:

require('./esm-snapshot-mutator')

Module Record is formed for ./esm-snapshot-mutator

require('./esm-snapshot')

Module Record is formed for ./esm-snapshot

Evaluation of ./esm-snapshot ends, module.exports === 1

Snapshot takes effect, Module Record saves as default = 1

Evaluation of ./esm-snapshot-mutator ends

The snapshot is taken before ./esm-snapshot-mutator changes the entry of require.cache.

This snapshot ensures the Module Namespace values of ./esm-snapshot is the same no matter when it is imported.

bmeck · 2017-11-08T12:48:31Z

lib/loader.js

+
+const Loader = require('internal/loader/Loader');
+
+module.exports = new Loader();


I am not comfortable exposing the loader when hooks are not even finished. Please remove this.

if its marked as experimental is that ok?

I'd still lean on no. I'm not entirely sure why you want it exposed if you want people to use hooks to mutate it.

@bmeck Node’s lack of APIs for its internals has always been a huge pain point. We should definitely expose the Loader at some point.

if you want people to use hooks to mutate it.

That makes sense if you think of the loader as Node’s way of loading its users’ modules from disk. Node definitely should have some kind of API for people to create and build modules. Think of it like the vm module, just more complicated because modules are more complicated than scripts.

Btw, this is going to conflict with @JacksonTian’s https://npm.im/loader

@devsnek I'd need to do a code review of it since I didn't design it for public exposure.

@devsnek is there a use case you are trying to get by doing this? I keep asking the question but don't understand right now.

I would like to expose it to allow for more control over node's loader, which will be a very important system moving forward. In the same way that require can be messed with, I think that loader should be able to be messed with. I agree that there should be control over what stuff can be messed with, but the loader having a public api is, in my opinion, a must have moving forward. I think that the hooks idea is good as a controlled system (and should remain in), but that it should also be possible to make your own loaders, change them up, etc. More to this point is why i suggested that at least exposing the class by default while allowing access to the instance with a flag would be a good choice to keep the system save and extensible at the same time.

I would need to understand various aspects of this. I see you want it, but don't see why except an opinion of "I think that loader should be able to be messed with". Various parts of mutating require like NODE_PATH and require.extensions were deprecated and I don't feel that saying require is mutable so the future should be mutable is a strong position. Exposing individual use cases like hooking into how specifier resolution works is very different from creating multiple disconnected loaders.

In particular, this exposure is problematic since the Module Map is not shared. I would not expose this without a clearer reasoning to need/want multiple completely disconnected loaders.

I also feel like the Realms API should be looked at to ensure we don't have problems if/when it arrives. The people who worked on the WHATWG Loader API are involved in that. It doesn't look to be exposing a Loader like object from the last few TC39 meetings on this topic.

If you can explain the concrete use case of having multiple loaders and explain how it does not suffer from the problems of mutation in the same way that require has that seems fine. Right now this seems too rushed and hasn't looked at compatibility or scope of what exposing it means.

"Messing with" the loader won't just have effects for you and your app, it could very very easily leak out and have effects on the ecosystem.

Whether this is a good idea ever is a separate discussion, but allowing this kind of experimentation prior to everything being stable and finished is, imo, a very dangerous and bad idea.

bmeck · 2017-11-08T17:27:47Z

lib/loader.js

+
+const Loader = require('internal/loader/Loader');
+
+module.exports = new Loader();


#16874 (comment) remains, commenting since default github UI is hiding it

bmeck · 2017-11-08T18:27:13Z

lib/internal/loader/Loader.js

-    }
+// A closure is used here because we really really really don't want users
+// touching the hooks without using `hook()`.
+function Loader(base = getURLStringForCwd()) {


Why not a class?

the comment explains

It can still be a class with the this.hook = in the constructor

devsnek · 2017-11-08T23:11:11Z

@bmeck how do requires inside cjs modules loaded with loader.import get linked as esm deps?
Can they just do the normal cjs require? (if this isn't possible then 16675 will have to get closed)

bmeck · 2017-11-08T23:35:54Z

@devsnek It has 2 parts:

When `import`ing CJS:

During HostResolveImportedModule the specifier is resolved. It is then transformed into a module record during the phase of Instantiate for the ModuleJob that is loading the CJS. This registers a function in the DymanicModule that is used during InnerModuleEvaluation. This function will call module._load during that time (which may have had behavior mutated by things like babel-node).

When `require`ing CJS:

After _load finished for a CJS module it checks to see if an entry is in the ModuleMap and adds it if it doesn't exist. It grabs a snapshot of module.exports using a regular property access to module.exports firing any getters that may be present.

This is not the most performant approach since we can delay actually creating the ESM record and incurring the parse cost of the DynamicModule. We could hold a reference to what the snapshot would be instead. Lacking that lazy behavior the main reason CJS load time suffers when using --experimental-modules.

devsnek · 2017-11-08T23:42:11Z

@bmeck i meant uh

// file.js
require('fs');

// main.mjs
import 'file.js';

when running link on file.js's ModuleWrap instance, fsshows up as a dep. shouldn't it be handled by require, not loader? this is separate from it getting added to module map.

bmeck · 2017-11-08T23:46:16Z

@devsnek it shouldn't. What is the output with NODE_DEBUG=esm node --experimental-modules main.mjs

devsnek · 2017-11-08T23:49:47Z

yea i tried that earlier, got really confused:

ESM 10912: Storing file:///main.mjs in ModuleMap
ESM 10912: Loading StandardModule file:///main.mjs
ESM 10912: creating ESM facade for file://file.js with exports: default
ESM 10912: Storing file:///file.js in ModuleMap
ESM 10912: Loading CJSModule file:///file.js

bmeck · 2017-11-08T23:53:27Z

Ah, it looks like

node/lib/internal/loader/ModuleRequest.js

Line 30 in d597317

debug(`Loading StandardModule ${url}`);

is logging at a different time than the other Module types. Thats a bug that should be fixed.

I don't see node:fs anywhere to imply the fs module was wrapped into an ESM.

devsnek · 2017-11-09T01:01:47Z

@bmeck on my latest commit i added buildAtInstantiate which should allow some more freedom with translating modules, but i'm having some issues.
using the example above, i get
Error: linking error, dependency promises must be resolved on instantiate
when calling this.module.instantiate() for main.mjs
they should all be resolved at that point, i think i just need another pair of eyes. could you take a look? (i've left some debug stuff in)

bnoordhuis

Not really a review, just a quick glance.

bnoordhuis · 2017-11-09T11:45:38Z

lib/internal/module.js

@@ -86,7 +86,7 @@ const builtinLibs = [
  'assert', 'async_hooks', 'buffer', 'child_process', 'cluster', 'crypto',
  'dgram', 'dns', 'domain', 'events', 'fs', 'http', 'http2', 'https', 'net',
  'os', 'path', 'perf_hooks', 'punycode', 'querystring', 'readline', 'repl',
-  'stream', 'string_decoder', 'tls', 'tty', 'url', 'util', 'v8', 'vm', 'zlib'
+  'stream', 'string_decoder', 'tls', 'tty', 'url', 'util', 'v8', 'vm', 'zlib',


Unrelated change?

bnoordhuis · 2017-11-09T11:46:01Z

test/es-module/test-esm-snapshot.mjs

@@ -1,5 +1,6 @@
 // Flags: --experimental-modules
 /* eslint-disable required-modules */
+


bmeck · 2017-11-09T15:13:26Z

lib/internal/loader/CreateDynamicModule.js

@@ -5,7 +5,7 @@ const debug = require('util').debuglog('esm');
 const ArrayJoin = Function.call.bind(Array.prototype.join);
 const ArrayMap = Function.call.bind(Array.prototype.map);

-const createDynamicModule = (exports, url = '', evaluate) => {
+function createDynamicModule(exports, url = '', evaluate) {


please change this back to an arrow function, it lacks a .prototype and protects against new

will do, also did you see #16874 (comment)

I did, but this PR is a lot of changes which makes me a bit slow to re-review so quickly

devsnek · 2017-11-09T17:49:05Z

as far as i can tell at this point, module_wrap wants to have the full module at instantiate time, not just the dependencies. is this safe to change? technically if the module has no deps, it shouldn't need the full module until evaluation time.

bmeck · 2017-11-09T17:52:41Z

@devsnek InnerModuleInstantiation requires the full Module since it does instantiate the hoistable declarations using ModuleDeclarationEnvironmentSetup

bmeck · 2017-11-09T17:53:47Z

I think export * from also would require the full source text as well... can't remember why though. Collisions?

devsnek · 2017-11-09T17:56:04Z

@bmeck but if there aren't any hoistable declarations there's no need to have the full module right?

bmeck · 2017-11-09T17:58:48Z

@devsnek you still need it whenever a parent is instantiating. The whole graph Instantiates prior to any evaluation. Not sure I understand

devsnek · 2017-11-09T18:03:23Z

being able to defer translation until ModuleJob#run would be a nice way to unblock #16675

bmeck · 2017-11-10T15:57:39Z

lib/internal/loader/Loader.js

+    // Use .bind() to avoid giving access to the Loader instance when it is
+    // called as this.resolver(...);
+    this._resolve = resolve.bind(null);
+    this._dynamicTranslate = dynamicTranslate;


bind(null) here too

guybedford · 2018-01-15T19:07:54Z

Note this still needs a documentation change that the "cjs" name in the loader format has been changed to "commonjs". //cc @devsnek

jdalton · 2018-01-15T19:14:24Z

I missed the "commonjs" bikeshed. Just curious, what's the reason for using a short form name for "esm" but a longer form name for "commonjs" instead of "cjs"?

jdalton · 2018-01-15T19:18:27Z

@devsnek

commonjs is still short, not really any reason to make it cjs; ecmascript modules is a bit longer

It's also smooshing case though as it's not "Commonjs". Using "cjs" avoids that and keeps symmetry with "esm" mode as both short name forms. Even aligns better with the other form, "json".

MylesBorins · 2018-02-20T21:11:10Z

This is not landing cleanly on v9.x, would someone be willing to open a backport?

devsnek · 2018-02-20T21:38:13Z

I can do that when I get home in a few hours. i should open a pr targeting the 9.x branch right?

MylesBorins · 2018-02-20T22:03:16Z

@devsnek nevermind, I got it to land after another backport landed!

PR-URL: #16874 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com>

MylesBorins · 2018-03-20T15:38:49Z

Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

devsnek · 2018-03-20T17:36:32Z

this sits on top of #18085, so i'm not sure

guybedford · 2018-04-04T13:23:37Z

If this is backported, it should be backported along with #18596.

This restores a broken and erroneously removed error, which was accidentially renamed to ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK (notice the "INTST" vs "INST") in 921fb84 (PR #16874) and then had documentation and implementation removed under the old name in 6e1c25c (PR #18857), as it appeared unused. This error code never worked or was documented under the mistyped name ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, so renaming it back to ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK is a semver-patch fix. Refs: #21440 Refs: #21470 Refs: #16874 Refs: #18857

This restores a broken and erroneously removed error, which was accidentially renamed to ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK (notice the "INTST" vs "INST") in 921fb84 (PR nodejs#16874) and then had documentation and implementation removed under the old name in 6e1c25c (PR nodejs#18857), as it appeared unused. This error code never worked or was documented under the mistyped name ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, so renaming it back to ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK is a semver-patch fix. Refs: nodejs#21440 Refs: nodejs#21470 Refs: nodejs#16874 Refs: nodejs#18857

ChALkeR · 2018-06-30T21:26:24Z

#21493 also should be included with any backport.

This restores a broken and erroneously removed error, which was accidentially renamed to ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK (notice the "INTST" vs "INST") in 921fb84 (PR #16874) and then had documentation and implementation removed under the old name in 6e1c25c (PR #18857), as it appeared unused. This error code never worked or was documented under the mistyped name ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK, so renaming it back to ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK is a semver-patch fix. Refs: #21440 Refs: #21470 Refs: #16874 Refs: #18857 PR-URL: #21493 Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 7, 2017

devsnek changed the title ~~loader: refactor loader and expose it as a module~~ loader: refactor loader and expose it as a builtin Nov 7, 2017

jasnell requested review from bmeck and addaleax November 8, 2017 09:51

bmeck reviewed Nov 8, 2017

View reviewed changes

bmeck suggested changes Nov 8, 2017

View reviewed changes

devsnek mentioned this pull request Nov 8, 2017

loader, docs, test: named exports from commonjs modules #16675

Closed

4 tasks

bmeck reviewed Nov 8, 2017

View reviewed changes

devsnek changed the title ~~loader: refactor loader and expose it as a builtin~~ loader: refactor loader Nov 8, 2017

bnoordhuis reviewed Nov 9, 2017

View reviewed changes

bmeck reviewed Nov 9, 2017

View reviewed changes

bmeck reviewed Nov 10, 2017

View reviewed changes

bmeck approved these changes Nov 10, 2017

View reviewed changes

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 18, 2018

guybedford mentioned this pull request Feb 6, 2018

esm: revert format name to "cjs" over "commonjs" #18596

Closed

3 tasks

MylesBorins added the backport-requested-v9.x label Feb 20, 2018

MylesBorins removed the backport-requested-v9.x label Feb 20, 2018

MylesBorins pushed a commit that referenced this pull request Feb 20, 2018

module: refactor loader

afc87c2

PR-URL: #16874 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com>

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

MylesBorins added backport-requested-v8.x labels Mar 20, 2018

This was referenced Jun 23, 2018

errors: fix ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK #21492

Closed

errors: fix ERR_MISSING_DYNAMIC_INSTANTIATE_HOOK #21493

Closed

		const debug = require('util').debuglog('esm');

		const kDynamicTranslate = Symbol('dynamicTranslate');


		const Loader = require('internal/loader/Loader');

		module.exports = new Loader();

		@@ -1,5 +1,6 @@
		// Flags: --experimental-modules
		/* eslint-disable required-modules */

loader: refactor loader #16874

loader: refactor loader #16874

Conversation

devsnek commented Nov 7, 2017 • edited

Checklist

Affected core subsystem(s)

devsnek commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Nov 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmeck commented Nov 8, 2017

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 Nov 8, 2017 • edited

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

addaleax Nov 8, 2017 • edited

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 Nov 8, 2017 • edited

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

devsnek commented Nov 8, 2017

bmeck commented Nov 8, 2017 • edited

When importing CJS:

When requireing CJS:

devsnek commented Nov 8, 2017 • edited

bmeck commented Nov 8, 2017

devsnek commented Nov 8, 2017

bmeck commented Nov 8, 2017

devsnek commented Nov 9, 2017 • edited

bnoordhuis 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

devsnek commented Nov 9, 2017 • edited

bmeck commented Nov 9, 2017

bmeck commented Nov 9, 2017

devsnek commented Nov 9, 2017

bmeck commented Nov 9, 2017

devsnek commented Nov 9, 2017 • edited

Choose a reason for hiding this comment

guybedford commented Jan 15, 2018

jdalton commented Jan 15, 2018 • edited

jdalton commented Jan 15, 2018 • edited

MylesBorins commented Feb 20, 2018

devsnek commented Feb 20, 2018

MylesBorins commented Feb 20, 2018

MylesBorins commented Mar 20, 2018

devsnek commented Mar 20, 2018 • edited

guybedford commented Apr 4, 2018

ChALkeR commented Jun 30, 2018

devsnek commented Nov 7, 2017 •

edited

devsnek Nov 8, 2017 •

edited

bmeck Nov 8, 2017 •

edited

addaleax Nov 8, 2017 •

edited

bmeck Nov 8, 2017 •

edited

bmeck commented Nov 8, 2017 •

edited

When `import`ing CJS:

When `require`ing CJS:

devsnek commented Nov 8, 2017 •

edited

devsnek commented Nov 9, 2017 •

edited

devsnek commented Nov 9, 2017 •

edited

devsnek commented Nov 9, 2017 •

edited

jdalton commented Jan 15, 2018 •

edited

jdalton commented Jan 15, 2018 •

edited

devsnek commented Mar 20, 2018 •

edited