Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module: add isPreloading indicator #36263

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Nov 25, 2020

Adds a module.isPreloading property that is true only during the
preload (-r) phase of Node.js bootstrap. This provides modules an
easy, non-hacky way of knowing if they are being loaded during preload.

For example, sample.js:

console.log(module.isPreloading);
$ node -r sample.js -pe ""
true
undefined
$ node sample.js
false

Signed-off-by: James M Snell jasnell@gmail.com

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

@jasnell jasnell added module Issues and PRs related to the module subsystem. semver-minor PRs that contain new features and should be released in the next minor version. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 25, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2020
@nodejs-github-bot
Copy link
Collaborator

doc/api/module.md Outdated Show resolved Hide resolved
@ShogunPanda
Copy link
Contributor

ShogunPanda commented Nov 26, 2020

Maybe it might be a little out of context for a single PR, but would it be possible to also add isMain or isEntrypoint (or whatever name you see fit) to also clearly state which was the main script launched with node?

@jasnell
Copy link
Member Author

jasnell commented Nov 26, 2020

@ShogunPanda ... There's already require.main that can be used to easily make that determination. Not sure adding an additional property helps.

doc/api/module.md Outdated Show resolved Hide resolved
Adds a `module.isPreloading` property that is `true` only during the
preload (`-r`) phase of Node.js bootstrap. This provides modules an
easy, non-hacky way of knowing if they are being loaded during preload.

Signed-off-by: James M Snell <jasnell@gmail.com>
jasnell added a commit that referenced this pull request Nov 30, 2020
Adds a `module.isPreloading` property that is `true` only during the
preload (`-r`) phase of Node.js bootstrap. This provides modules an
easy, non-hacky way of knowing if they are being loaded during preload.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36263
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Nov 30, 2020

Landed in 8ff2501

@jasnell jasnell closed this Nov 30, 2020
@Trott
Copy link
Member

Trott commented Nov 30, 2020

@nodejs/modules-active-members If there are any concerns about this, including whether isPreloading is the right name or whether module is the right place to put this property, let's tag this appropriately so it doesn't go out in a release before those things are hashed out. It seems good/correct to me, but I'll certainly defer to the experts on this.

@bmeck
Copy link
Member

bmeck commented Nov 30, 2020

I've often thought we might instead want to use conditions to allow this, i don't know where to put a property like this PR does on an ESM thing (JS or WASM).

@ljharb
Copy link
Member

ljharb commented Dec 1, 2020

Is this something that can be polyfilled, since it won’t be backportable earlier than node 14?

@jkrems
Copy link
Contributor

jkrems commented Dec 1, 2020

Is this an attribute of the execution phase more so than of the current file? If so, could it be something imported, e.g. from module or process? That should work for both ESM and CJS.

@ljharb
Copy link
Member

ljharb commented Dec 1, 2020

Or even simpler, a global that only exists during this phase.

@jasnell
Copy link
Member Author

jasnell commented Dec 1, 2020

I've often thought we might instead want to use conditions to allow this, i don't know where to put a property like this PR does on an ESM thing (JS or WASM).

Unless we had preloads figured out for ESM, it doesn't really matter. If preloads did support it, a property in import.meta would make sense.

Is this something that can be polyfilled, since it won’t be backportable earlier than node 14?

Why wouldn't it be backportable earlier than 14? And no, it can't be polyfilled.

Is this an attribute of the execution phase more so than of the current file? If so, could it be something imported, e.g. from module or process? That should work for both ESM and CJS.

It's an attribute of the execution phase that is not relevant to ESM because preloads do not support ESM.

@ljharb
Copy link
Member

ljharb commented Dec 1, 2020

@jasnell i was under the impression that v12.20 was the last expected minor in v12 before it goes into maintenance.

Preloads will one day support ESM, so it seems prudent to aim for a mechanism that's the same in both CJS and ESM.

@jasnell
Copy link
Member Author

jasnell commented Dec 1, 2020

import { isPreloading } from 'module' 

... Should work easily enough if preloads ever actually support ESM?

There may be process reasons why it's not backported but there's no technical reason why it can't be backported. It can't be polyfilled tho.

@ljharb
Copy link
Member

ljharb commented Dec 1, 2020

What's the advantage to making it importable when it's global state? Global state is usually accessed via "a global".

@jasnell
Copy link
Member Author

jasnell commented Dec 1, 2020

I haven't made it importable. I'm just saying that it could be if necessary. The pattern I suggested is consistent with what we've done elsewhere also (see import { isMainThread } from 'worker_threads' for instance. If preloads ever actually do support ESM, the currently already landed mechanism would work consistently for both CJS and ESM. Unless someone can articulate why it wouldn't work for ESM, I'm having a hard time understanding why we're having this discussion after it's already landed.

@ljharb
Copy link
Member

ljharb commented Dec 1, 2020

We're having the discussion after it landed because a modules feature landed without anyone on the modules github team being pinged, as is customary for any feature that has a stakeholder github team.

I think you're right that it would work fine for ESM - I'm mainly just asking why "a global" wasn't considered, given that it's global state.

@jasnell
Copy link
Member Author

jasnell commented Dec 1, 2020

I'm mainly just asking why "a global" wasn't considered

I'm still confused. It was considered.

For instance...

$ node -pe "module.isPreloading" 

Feels pretty global. We even list module as a global (even if it only appears as such and isn't really one).

We do have a tradition of treating new globals as semver-major, which I wanted to avoid here. Dropping this off the existing pseudo-global module made the most sense to keep is semver-minor (and therefore backportable) and easily adaptable to ESM should that hypothetical future ever happen.

And as for pinging the modules team, I had (apparently incorrectly) assumed that the CODEOWNERS file would cover notifying the modules team of changes in the 'internal/modules/cjs/* subfolder but I guess the existing rules didn't work as they were supposed to? May be good for someone to look into that.

@DerekNonGeneric DerekNonGeneric self-assigned this Dec 1, 2020
@ljharb
Copy link
Member

ljharb commented Dec 1, 2020

Thanks, that answers my question (and I'm convinced; new globals are indeed semver-major, and module is indeed effectively a CJS global, and import 'module' in ESM is totally reasonable).

fwiw, i'm not implying a mistake was made or anything intentional was done or that a revert would be reasonable, it's just that the post-merge ping is the reason it's being discussed now :-) thanks for bearing with me.

@guybedford
Copy link
Contributor

We should ideally make sure that the live binding for import { isPreloading } from 'module' works here. See the implementation of syncESMBuiltinExports for how to do this.

@jkrems
Copy link
Contributor

jkrems commented Dec 2, 2020

Alternatively, getProcessReadyState() which would remove the need for the live binding. But either would work. :)

danielleadams pushed a commit that referenced this pull request Dec 7, 2020
Adds a `module.isPreloading` property that is `true` only during the
preload (`-r`) phase of Node.js bootstrap. This provides modules an
easy, non-hacky way of knowing if they are being loaded during preload.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36263
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@danielleadams danielleadams mentioned this pull request Dec 7, 2020
danielleadams added a commit that referenced this pull request Dec 7, 2020
PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)
cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 8, 2020
Adds a `module.isPreloading` property that is `true` only during the
preload (`-r`) phase of Node.js bootstrap. This provides modules an
easy, non-hacky way of knowing if they are being loaded during preload.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#36263
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
danielleadams added a commit that referenced this pull request Dec 9, 2020
PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)
danielleadams added a commit that referenced this pull request Dec 9, 2020
PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)
jasnell pushed a commit that referenced this pull request Jan 18, 2021
Fixes: #36775

PR-URL: #36944
Refs: #36263
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit to aduh95/node that referenced this pull request Jan 18, 2021
Adds a `module.isPreloading` property that is `true` only during the
preload (`-r`) phase of Node.js bootstrap. This provides modules an
easy, non-hacky way of knowing if they are being loaded during preload.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#36263
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
aduh95 added a commit to aduh95/node that referenced this pull request Jan 18, 2021
Fixes: nodejs#36775

PR-URL: nodejs#36944
Refs: nodejs#36263
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno pushed a commit that referenced this pull request Jan 22, 2021
Fixes: #36775

PR-URL: #36944
Refs: #36263
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno pushed a commit that referenced this pull request Jan 25, 2021
Fixes: #36775

PR-URL: #36944
Refs: #36263
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 25, 2021
Adds a `module.isPreloading` property that is `true` only during the
preload (`-r`) phase of Node.js bootstrap. This provides modules an
easy, non-hacky way of knowing if they are being loaded during preload.

Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36263
Backport-PR-URL: #36988
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Apr 25, 2021
Fixes: #36775

PR-URL: #36944
Backport-PR-URL: #36988
Refs: #36263
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.