src: restructure modules and bootstrappers #19177

joyeecheung · 2018-03-06T19:15:53Z

Refs: #19112

src: put bootstrappers in lib/internal/bootstrap/

Create lib/internal/bootstrap/ and put bootstrappers there:

Before:

lib/internal
├── ...
├── bootstrap_loaders.js
└── bootstrap_node.js

After:

lib/internal
├── ...
└── bootstrap
    ├── loaders.js
    └── node.js

lib: restructure cjsm and esm loaders

Create lib/internal/modules and restructure the module loaders
to make the purpose of those files clearer.

Also make it clear in the code that the object exported by
lib/internal/modules/cjsm.js is CJSModule instead of the
ambiguous Module.

Before (three files named module.js without any indication of
what they are implementing):

lib
├── ...
├── internal
│       ├── process
│       │     └── module.js
│       ├── loaders
│       │     ├── CreateDynamicModule.js
│       │     ├── DefaultResolve.js
│       │     ├── Loader.js
│       │     ├── ModuleJob.js
│       │     ├── ModuleMap.js
│       │     └── Translators.js
│       └── module.js
└── module.js

After (file names indicate the implemented module style):

├── ...
├── internal
│       ├── ...
│       ├── process
│       │     └── esm_loader.js
│       └── modules
│              ├── cjs
│              │     ├── helpers.js
│              │     └── loader.js
│              └── esm
│                    ├── CreateDynamicModule.js
│                    ├── DefaultResolve.js
│                    ├── Loader.js
│                    ├── ModuleJob.js
│                    ├── ModuleMap.js
│                    └── Translators.js
└── module.js

Checklist

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

joyeecheung · 2018-03-06T19:17:25Z

cc @devsnek

addaleax · 2018-03-06T19:22:33Z

lib/module.js

-
-// backwards compatibility
-Module.Module = Module;
+module.exports = require('internal/modules/cjsm');


I don’t think git recognizes this as a file move, so it won’t say anything about merge conflicts? That might be okay, we don’t have many PRs against this file open/to backport, I think…

@addaleax If we don't require that each commit must pass the test, we can delete the file first (so it will be recognized as a move) and then add the redirection later.

devsnek

internal/modules/helper.js should definitely be moved to internal/modules/cjs/helper.js and internal/modules/cjsm.js should be internal/modules/cjs.js or tbh i think it was fine in module.js

aside from that i worry about packages like trace.js clarify possibly getting killed by at Module._compile (module.js) becoming at Module._compile (internal/modules/cjsm.js) but i haven't looked into that too deeply, so it might not be an issue

joyeecheung · 2018-03-06T19:45:11Z

@devsnek I understand that the current internal/modules/helper.js can be renamed as internal/modules/cjs/helper.js (although my thinking is that we can add common helpers there so it does not need to be in any subdirectory. Things like stripBOM aren't really tied to any loader implementations).

Bikeshedding question: why does it need to be cjs instead of cjsm? CommonJS is not the equivalent of CommonJS Modules, just like ECMAScript is not the equivalent of ECMAScript Modules (although we are not respecting the original specs in things like assert either).

aside from that i worry about packages like trace possibly getting killed by at Module._compile (module.js) becoming at Module._compile (internal/modules/cjsm.js) but i haven't looked into that too deeply, so it might not be an issue

cc @AndreasMadsen although I don't think we should encourage user land modules to depend on our source positions..

devsnek · 2018-03-06T19:54:19Z

@joyeecheung

Things like stripBOM aren't really tied to any loader implementations).

perhaps a better alternative would be moving that require building stuff out of the file then.

Bikeshedding question: why does it need to be cjs instead of cjsm?

everywhere else in the codebase that we shorten it we call it cjs, that's my only reasoning for it.

joyeecheung · 2018-03-06T19:54:41Z

I also realize that there is the internal/process/module.js that should probably be renamed to internal/process/esm_loader.js since it's specific to setting up the ESM loader.

joyeecheung · 2018-03-06T20:17:04Z

everywhere else in the codebase that we shorten it we call it cjs, that's my only reasoning for it.

Looks like this has already been documented in the resolve hook formats..unfortunate, but probably better name the directory as cjs as well for consistency

joyeecheung · 2018-03-06T20:22:49Z

aside from that i worry about packages like trace.js clarify possibly getting killed by at Module._compile (module.js) becoming at Module._compile (internal/modules/cjsm.js) but i haven't looked into that too deeply, so it might not be an issue

I ran the test of clarify, it seems to be able to exclude all modules starting with internal so that should not be a problem.

joyeecheung · 2018-03-06T21:07:40Z

@devsnek I've changed the structure, it now looks like this:

lib
├── ...
├── internal
│       ├── ...
│       └── modules
│              ├── cjs
│              │     ├── helpers.js
│              │     └── loader.js
│              └── esm
│                    ├── CreateDynamicModule.js
│                    ├── DefaultResolve.js
│                    ├── Loader.js
│                    ├── ModuleJob.js
│                    ├── ModuleMap.js
│                    └── Translators.js
└── module.js

PTAL, thanks!

devsnek

nit: const internalModule = require('internal/modules/cjs/helpers'); is a bit weird to look at, maybe the file should be called internalModule?

joyeecheung · 2018-03-06T21:21:27Z

@addaleax BTW, the file mode conflicts can be solved with this:

git format-patch <sha>~1...<sha>
# suppose the output is `0001-test.patch`
sed -i '' -E "s/lib\/module.js/lib\/internal\/modules\/cjs\/loader.js/" 0001-test.patch
git am -3 0001-test.patch

If there are future backports (although given our stability concerns around the CJS loader implementation that should be rare), we just need to replace the lib/module.js in the email-formatted patch with lib/internal/module/cjs/loader.js and apply it with git am -3

joyeecheung · 2018-03-06T21:27:02Z

@devsnek I think it's the variable that should be renamed to helpers? I believe they were named internalModule because the file was internal/module, not the other way around..

joyeecheung · 2018-03-06T21:34:22Z

@devsnek Hmm, so the only const internalModule = require('internal/modules/cjs/helpers'); left are the test test-module-require-depth and its fixtures..I changed to use destructuring instead since they only need the const values of requireDepth.

joyeecheung · 2018-03-06T21:35:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13546/

joyeecheung · 2018-03-07T18:24:25Z

Rebased. New CI: https://ci.nodejs.org/job/node-test-pull-request/13574/

AndreasMadsen · 2018-03-07T20:08:23Z

aside from that i worry about packages like trace possibly getting killed by at Module._compile (module.js) becoming at Module._compile (internal/modules/cjsm.js) but i haven't looked into that too deeply, so it might not be an issue

cc @AndreasMadsen although I don't think we should encourage user land modules to depend on our source positions.

It should still work. The name.startsWith('internal') check will match internal/modules/cjsm.js.

apapirovski · 2018-03-08T11:57:36Z

deps/node-inspect/lib/internal/inspect_repl.js

@@ -86,7 +86,9 @@ function extractFunctionName(description) {

 const NATIVES = process.binding('natives');
 function isNativeUrl(url) {
-  return url.replace('.js', '') in NATIVES || url === 'bootstrap_node.js';
+  return url.replace('.js', '') in NATIVES ||


Should a PR be opened against https://github.com/nodejs/node-inspect before this lands so it doesn't get forgotten?

Also, strictly speaking, this change should either be in the same or preceding commit to the actual filename change (rather than after it), as otherwise node-inspect is broken.

(I just skimmed the PR so I could be totally off base.)

@apapirovski I just realized this is not necessary since

> Object.keys(process.binding('natives')) [ 'internal/bootstrap/loaders', 'internal/bootstrap/node', ...

node-inspect only needed to test 'bootstrap_node.js' because it was internal/bootstrap_node.js but it didn't get named that way when being compiled.

MylesBorins · 2018-03-20T11:15:28Z

Should this be backported to v9.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.

Otherwise the debug log output might be mixed up with the expected errors and the assertion matching the error message would fail. PR-URL: #19177 Refs: #19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

joyeecheung · 2018-03-20T12:55:45Z

@MylesBorins Backport opened in #19374 , somehow I forgot to reference the PR from there so it did not show up in this thread

Otherwise the debug log output might be mixed up with the expected errors and the assertion matching the error message would fail. PR-URL: nodejs#19177 Refs: nodejs#19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Create `lib/internal/bootstrap/` and put bootstrappers there: Before: ``` lib/internal ├── ... ├── bootstrap_loaders.js └── bootstrap_node.js ``` After: ``` lib/internal ├── ... └── bootstrap ├── loaders.js └── node.js ``` PR-URL: nodejs#19177 Refs: nodejs#19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Create `lib/internal/modules` and restructure the module loaders to make the purpose of those files clearer. Also make it clear in the code that the object exported by `lib/internal/modules/cjs/loader.js` is `CJSModule` instead of the ambiguous `Module`. Before: ``` lib ├── ... ├── internal │ ├── loaders │ │ ├── CreateDynamicModule.js │ │ ├── DefaultResolve.js │ │ ├── Loader.js │ │ ├── ModuleJob.js │ │ ├── ModuleMap.js │ │ ├── ModuleWrap.js │ │ └── Translators.js │ └── module.js └── module.js ``` After: ``` lib ├── ... ├── internal │ ├── ... │ └── modules │ ├── cjs │ │ ├── helpers.js │ │ └── loader.js │ └── esm │ ├── CreateDynamicModule.js │ ├── DefaultResolve.js │ ├── Loader.js │ ├── ModuleJob.js │ ├── ModuleMap.js │ └── Translators.js └── module.js # deleted in this commit to work with git file mode ``` PR-URL: nodejs#19177 Refs: nodejs#19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

The previous commit deleted lib/module.js so that git recognize the file move `lib/module.js` -> `lib/internal/modules/cjs/loader.js`. This commit add the redirection back. PR-URL: nodejs#19177 Refs: nodejs#19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Create `lib/internal/bootstrap/` and put bootstrappers there: Before: ``` lib/internal ├── ... ├── bootstrap_loaders.js └── bootstrap_node.js ``` After: ``` lib/internal ├── ... └── bootstrap ├── loaders.js └── node.js ``` Backport-PR-URL: #19374 PR-URL: #19177 Refs: #19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Otherwise the debug log output might be mixed up with the expected errors and the assertion matching the error message would fail. Backport-PR-URL: #19374 PR-URL: #19177 Refs: #19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Create `lib/internal/modules` and restructure the module loaders to make the purpose of those files clearer. Also make it clear in the code that the object exported by `lib/internal/modules/cjs/loader.js` is `CJSModule` instead of the ambiguous `Module`. Before: ``` lib ├── ... ├── internal │ ├── loaders │ │ ├── CreateDynamicModule.js │ │ ├── DefaultResolve.js │ │ ├── Loader.js │ │ ├── ModuleJob.js │ │ ├── ModuleMap.js │ │ ├── ModuleWrap.js │ │ └── Translators.js │ └── module.js └── module.js ``` After: ``` lib ├── ... ├── internal │ ├── ... │ └── modules │ ├── cjs │ │ ├── helpers.js │ │ └── loader.js │ └── esm │ ├── CreateDynamicModule.js │ ├── DefaultResolve.js │ ├── Loader.js │ ├── ModuleJob.js │ ├── ModuleMap.js │ └── Translators.js └── module.js # deleted in this commit to work with git file mode ``` Backport-PR-URL: #19374 PR-URL: #19177 Refs: #19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

The previous commit deleted lib/module.js so that git recognize the file move `lib/module.js` -> `lib/internal/modules/cjs/loader.js`. This commit add the redirection back. Backport-PR-URL: #19374 PR-URL: #19177 Refs: #19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Otherwise the debug log output might be mixed up with the expected errors and the assertion matching the error message would fail. PR-URL: nodejs#19177 Refs: nodejs#19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Create `lib/internal/bootstrap/` and put bootstrappers there: Before: ``` lib/internal ├── ... ├── bootstrap_loaders.js └── bootstrap_node.js ``` After: ``` lib/internal ├── ... └── bootstrap ├── loaders.js └── node.js ``` PR-URL: nodejs#19177 Refs: nodejs#19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

The current caching logic broke by [0] because it used destructuring on the module arguments. Since the exported property is a primitive counting it up or down would not have any effect anymore in the module that required that property. The original implementation would cache all stat calls caused during bootstrap. Afterwards it would clear the cache and lazy require calls during runtime would create a new cascading cache for the then loaded modules and clear the cache again. This behavior is now restored. This is difficult to test without exposing a lot of information and therfore the existing tests have been removed (as they could not detect the issue). With the broken implementation it caused each module compilation to reset the cache and therefore minimizing the effect drastically. [0] nodejs#19177

The current caching logic broke by [0] because it used destructuring on the module arguments. Since the exported property is a primitive counting it up or down would not have any effect anymore in the module that required that property. The original implementation would cache all stat calls caused during bootstrap. Afterwards it would clear the cache and lazy require calls during runtime would create a new cascading cache for the then loaded modules and clear the cache again. This behavior is now restored. This is difficult to test without exposing a lot of information and therfore the existing tests have been removed (as they could not detect the issue). With the broken implementation it caused each module compilation to reset the cache and therefore minimizing the effect drastically. [0] nodejs#19177 PR-URL: nodejs#26266 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

The current caching logic broke by [0] because it used destructuring on the module arguments. Since the exported property is a primitive counting it up or down would not have any effect anymore in the module that required that property. The original implementation would cache all stat calls caused during bootstrap. Afterwards it would clear the cache and lazy require calls during runtime would create a new cascading cache for the then loaded modules and clear the cache again. This behavior is now restored. This is difficult to test without exposing a lot of information and therfore the existing tests have been removed (as they could not detect the issue). With the broken implementation it caused each module compilation to reset the cache and therefore minimizing the effect drastically. [0] #19177 PR-URL: #26266 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 6, 2018

addaleax reviewed Mar 6, 2018

View reviewed changes

joyeecheung force-pushed the rename-modules branch from d265367 to e907edc Compare March 6, 2018 19:23

devsnek suggested changes Mar 6, 2018

View reviewed changes

joyeecheung force-pushed the rename-modules branch 5 times, most recently from 0e24abf to cf967f4 Compare March 6, 2018 21:05

devsnek approved these changes Mar 6, 2018

View reviewed changes

joyeecheung added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. labels Mar 6, 2018

joyeecheung mentioned this pull request Mar 6, 2018

src: bootstrap internals loaders before bootstrapping Node.js #19112

Closed

2 tasks

joyeecheung force-pushed the rename-modules branch from 1ec41df to 2633cb7 Compare March 7, 2018 18:23

apapirovski reviewed Mar 8, 2018

View reviewed changes

joyeecheung force-pushed the rename-modules branch 2 times, most recently from 27f010a to 3ebaa39 Compare March 8, 2018 16:30

joyeecheung mentioned this pull request Mar 20, 2018

[9.x] backport 19112, 19177 (bootstrapper & loaders reafactor) #19374

Closed

targos mentioned this pull request Apr 4, 2018

v9.11.0 proposal #19799

Merged

BridgeAR mentioned this pull request May 1, 2018

[9.x] Backport a lot of things #20448

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: restructure modules and bootstrappers #19177

src: restructure modules and bootstrappers #19177

joyeecheung commented Mar 6, 2018 •

edited

Loading

joyeecheung commented Mar 6, 2018

addaleax Mar 6, 2018

joyeecheung Mar 6, 2018

devsnek left a comment •

edited

Loading

joyeecheung commented Mar 6, 2018 •

edited

Loading

devsnek commented Mar 6, 2018

joyeecheung commented Mar 6, 2018

joyeecheung commented Mar 6, 2018

joyeecheung commented Mar 6, 2018

joyeecheung commented Mar 6, 2018

devsnek left a comment

joyeecheung commented Mar 6, 2018 •

edited

Loading

joyeecheung commented Mar 6, 2018

joyeecheung commented Mar 6, 2018

joyeecheung commented Mar 6, 2018

joyeecheung commented Mar 7, 2018

AndreasMadsen commented Mar 7, 2018

apapirovski Mar 8, 2018

joyeecheung Mar 8, 2018 •

edited

Loading

MylesBorins commented Mar 20, 2018

joyeecheung commented Mar 20, 2018

src: restructure modules and bootstrappers #19177

src: restructure modules and bootstrappers #19177

Conversation

joyeecheung commented Mar 6, 2018 • edited Loading

src: put bootstrappers in lib/internal/bootstrap/

lib: restructure cjsm and esm loaders

Checklist

joyeecheung commented Mar 6, 2018

addaleax Mar 6, 2018

Choose a reason for hiding this comment

joyeecheung Mar 6, 2018

Choose a reason for hiding this comment

devsnek left a comment • edited Loading

Choose a reason for hiding this comment

joyeecheung commented Mar 6, 2018 • edited Loading

devsnek commented Mar 6, 2018

joyeecheung commented Mar 6, 2018

joyeecheung commented Mar 6, 2018

joyeecheung commented Mar 6, 2018

joyeecheung commented Mar 6, 2018

devsnek left a comment

Choose a reason for hiding this comment

joyeecheung commented Mar 6, 2018 • edited Loading

joyeecheung commented Mar 6, 2018

joyeecheung commented Mar 6, 2018

joyeecheung commented Mar 6, 2018

joyeecheung commented Mar 7, 2018

AndreasMadsen commented Mar 7, 2018

apapirovski Mar 8, 2018

Choose a reason for hiding this comment

joyeecheung Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

MylesBorins commented Mar 20, 2018

joyeecheung commented Mar 20, 2018

joyeecheung commented Mar 6, 2018 •

edited

Loading

devsnek left a comment •

edited

Loading

joyeecheung commented Mar 6, 2018 •

edited

Loading

joyeecheung commented Mar 6, 2018 •

edited

Loading

joyeecheung Mar 8, 2018 •

edited

Loading