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

joyeecheung · 2018-03-15T15:23:19Z

src: move internal loaders out of bootstrap_node.js

Moves the creation of process.binding(), process._linkedBinding()
internalBinding() and NativeModule into a separate file
lib/internal/bootstrap_loaders.js, and documents them there.
This file will be compiled and run before bootstrap_node.js, which
means we now bootstrap the internal module & binding system before
actually bootstrapping Node.js.
Rename the special ID that can be used to require NativeModule
as internal/bootstrap_loaders since it is setup there. Also put
internalBinding in the object exported by NativeModule.require
instead of putting it inside the NativeModule.wrapper
Use the original getBinding() to get the source code of native
modules instead of getting it from process.binding('native')
so that users cannot fake native modules by modifying the binding
object.
Names the bootstrapping functions so their names show up
in the stack trace.

test: remove NODE_DEBUG in global module loading test

Otherwise the debug log output might be mixed up with
the expected errors and the assertion matching the error
message would fail.

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 cjs 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/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

lib: add back lib/module.js redirection

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.

Refs #19112
Refs #19177

targos · 2018-03-24T10:23:31Z

@joyeecheung Can you please rebase? I promise to check this as soon as possible.

- Moves the creation of `process.binding()`, `process._linkedBinding()` `internalBinding()` and `NativeModule` into a separate file `lib/internal/bootstrap_loaders.js`, and documents them there. This file will be compiled and run before `bootstrap_node.js`, which means we now bootstrap the internal module & binding system before actually bootstrapping Node.js. - Rename the special ID that can be used to require `NativeModule` as `internal/bootstrap_loaders` since it is setup there. Also put `internalBinding` in the object exported by `NativeModule.require` instead of putting it inside the `NativeModule.wrapper` - Use the original `getBinding()` to get the source code of native modules instead of getting it from `process.binding('native')` so that users cannot fake native modules by modifying the binding object. - Names the bootstrapping functions so their names show up in the stack trace. PR-URL: nodejs#19112 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>

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 <me@gus.host> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

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 <me@gus.host> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

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 <me@gus.host> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

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 <me@gus.host> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

joyeecheung · 2018-03-30T17:19:12Z

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

- Moves the creation of `process.binding()`, `process._linkedBinding()` `internalBinding()` and `NativeModule` into a separate file `lib/internal/bootstrap_loaders.js`, and documents them there. This file will be compiled and run before `bootstrap_node.js`, which means we now bootstrap the internal module & binding system before actually bootstrapping Node.js. - Rename the special ID that can be used to require `NativeModule` as `internal/bootstrap_loaders` since it is setup there. Also put `internalBinding` in the object exported by `NativeModule.require` instead of putting it inside the `NativeModule.wrapper` - Use the original `getBinding()` to get the source code of native modules instead of getting it from `process.binding('native')` so that users cannot fake native modules by modifying the binding object. - Names the bootstrapping functions so their names show up in the stack trace. Backport-PR-URL: #19374 PR-URL: #19112 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>

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 <me@gus.host> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

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 <me@gus.host> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

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 <me@gus.host> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

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 <me@gus.host> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

targos · 2018-04-04T14:22:26Z

Thanks. Landed in 5e90fc6...8e44011

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v9.x labels Mar 15, 2018

joyeecheung force-pushed the backport-refactors-to-v9.x branch from 123623d to aa9976c Compare March 15, 2018 16:01

MylesBorins force-pushed the v9.x-staging branch 4 times, most recently from d457b9d to 03c321a Compare March 20, 2018 11:56

joyeecheung mentioned this pull request Mar 20, 2018

src: restructure modules and bootstrappers #19177

Closed

3 tasks

MylesBorins force-pushed the v9.x-staging branch 2 times, most recently from 305fe4c to 4844a26 Compare March 28, 2018 16:23

joyeecheung added 5 commits March 31, 2018 00:35

joyeecheung force-pushed the backport-refactors-to-v9.x branch from aa9976c to f0d9722 Compare March 30, 2018 17:18

targos closed this Apr 4, 2018

joyeecheung mentioned this pull request May 2, 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

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

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

joyeecheung commented Mar 15, 2018 •

edited

Loading

targos commented Mar 24, 2018

joyeecheung commented Mar 30, 2018

targos commented Apr 4, 2018

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

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

Conversation

joyeecheung commented Mar 15, 2018 • edited Loading

src: move internal loaders out of bootstrap_node.js

test: remove NODE_DEBUG in global module loading test

src: put bootstrappers in lib/internal/bootstrap/

lib: restructure cjs and esm loaders

lib: add back lib/module.js redirection

targos commented Mar 24, 2018

joyeecheung commented Mar 30, 2018

targos commented Apr 4, 2018

joyeecheung commented Mar 15, 2018 •

edited

Loading