lib: expose primordials object by aduh95 · Pull Request #36872 · nodejs/node

aduh95 · 2021-01-10T21:31:55Z

Provide a way to expose the primordials on the global object. This is useful to run Node.js internals modules to help with Node.js core development.

$ node --expose-internals -r internal/test/binding lib/fs.js(node:5299) internal/test/binding: These APIs are for internal testing only. Do not use them.(Use `node --trace-warnings ...` to show where the warning was created)

devsnek · 2021-01-10T23:38:35Z

Can you move this to lib/internal/test/binding.js? That way we get the warning and it's neatly wrapped up with --expose-internals.

aduh95 · 2021-01-10T23:58:16Z

Can you move this to lib/internal/test/binding.js? That way we get the warning and it's neatly wrapped up with --expose-internals.

@devsnek Note that the current implementation requires to use two flags: --expose-internals and --expose-primordials. You are suggesting to remove --expose-primordials flag and keep --expose-internals only, correct? Or do you mean something else?

targos · 2021-01-11T06:33:23Z

I don't really like this solution for the referenced issue. The fact that we use built-in functions like Date.now somewhere in core is an implementation detail and should never be relied on by userland modules

bmeck · 2021-01-11T17:10:55Z

A lot of the purpose in making the slow migration to primordials is exactly to prevent userland mutation of core behaviors through side channels like this. I would be fine exposing primordials somehow since things like the demand to move mime from #21128 relies on being able to use them from a 3rd party. For now it seems the goal of the linked issue is just to alter what the generated Date header is. I think this just means that we need to expose a way to solve that issue, not to make primordials mutable and user accessible.

bmeck

primordials was introduced exactly to prevent mutating the internals of node core and to allow robustness in the core. I don't think this approach to solve the referenced issue is the way we should approach it.

bmeck · 2021-01-11T17:24:06Z

It looks like @sinonjs/fake-timers is in the 4M+ weekly downloads range, I think we should just solve the needed use case as exposing this flag would definitely not be niche in light of that size of usage and thus somewhat invalidate core's robustness goals.

addaleax · 2021-01-11T20:45:38Z

@bmeck is right – this PR would basically make having primordials pointless (which, yay, I think removing them would be a great idea, but I don’t think that’s happening). However, I’m commenting because I saw the issue title and thought “hey, maybe this is just exposing the deep-frozen object as-is”, which I think I’d like because it would help with Node.js core development (it answers questions like “What is in the primordials object” and “What happens when I run this piece of Node.js core source code”) and wouldn’t really hurt anyone.

ExE-Boss · 2021-01-13T15:20:40Z

Actually, this won’t work the way it’s intended due to the way the primordials object is used.

This is because all code that uses the primordials object destructures the primordials from the primordials object.

aduh95 · 2021-01-14T15:03:05Z

Thank you everyone for the reviews, I've rolled back the "altering primordials" part, this PR is now only exposing the primordials object on the global scope as @addaleax suggested. PTAL.

Note to whoever lands this: the commit message will need to be amended before landing to remove the reference to sinonjs/fake-timers#344.

bmeck · 2021-01-14T15:09:51Z

I do think we can address the timer issue somewhere else; even if we didn't use primordials, early destructuring would have broken that as well like @ExE-Boss points out.

nodejs-github-bot · 2021-01-14T17:16:46Z

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

nodejs-github-bot · 2021-01-14T23:08:28Z

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

aduh95 · 2021-01-15T14:00:50Z

Based on feedback shared by several persons, I've updated this PR to remove the --expose-primordials flag. Instead, this add an export to the existing internal/test/binding module to access the primordials object, and, when the module is preloaded with --require flag, expose the internal objects to the global. PTAL!

The intended use of this feature is to use --expose-internals in conjunction with --require internal/test/binding to be able to run an internal module.

$ node lib/fs.js…/node/lib/fs.js:51} = primordials; ^ReferenceError: primordials is not defined at Object.<anonymous> (…/node/lib/fs.js:51:5) at Module._compile (node:internal/modules/cjs/loader:1108:14) at Object.Module._extensions..js (node:internal/modules/cjs/loader:1137:10) at Module.load (node:internal/modules/cjs/loader:973:32) at Function.Module._load (node:internal/modules/cjs/loader:813:14) at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:12) at node:internal/main/run_main_module:17:47
$ node --expose-internals -r internal/test/binding lib/fs.js(node:5299) internal/test/binding: These APIs are for internal testing only. Do not use them.(Use `node --trace-warnings ...` to show where the warning was created)

aduh95 · 2021-01-15T14:03:16Z

On a side note, we might want to rename internal/test/binding to something else, but I can't think of a good name. If you have any, please throw them out there :)

nodejs-github-bot · 2021-01-15T23:26:42Z

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

mscdex · 2021-01-16T11:08:58Z

Where do we use the --require/global feature? Shouldn't it be enough to require() the internal module from within tests?

aduh95 · 2021-01-16T12:03:42Z

Where do we use the --require/global feature?

Not sure what you mean, module.isPreloading has been introduced in v15.4.0, so it's a rather new pattern.

Shouldn't it be enough to require() the internal module from within tests?

The idea is to give the ability for a node binary to run an internal module without modifying it. require still works as the module still exports the object, and if you prefer to use that, go for it.

nodejs-github-bot · 2021-01-16T15:07:36Z

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

nodejs-github-bot · 2021-01-16T17:12:52Z

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

nodejs-github-bot · 2021-01-17T22:32:56Z

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

Expose the internal `primordials` object to help with Node.js core development. ```console $ node --expose-internals -r internal/test/binding lib/fs.js (node:5299) internal/test/binding: These APIs are for internal testing only. Do not use them. (Use `node --trace-warnings ...` to show where the warning was created) ``` PR-URL: nodejs#36872 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

aduh95 · 2021-01-18T10:44:54Z

Landed in 983e922

Expose the internal `primordials` object to help with Node.js core development. ```console $ node --expose-internals -r internal/test/binding lib/fs.js (node:5299) internal/test/binding: These APIs are for internal testing only. Do not use them. (Use `node --trace-warnings ...` to show where the warning was created) ``` PR-URL: #36872 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 10, 2021

mscdex reviewed Jan 10, 2021
View reviewed changes

Comment threadtest/parallel/test-primordials-restricted-access.js Outdated

mscdex reviewed Jan 10, 2021
View reviewed changes

Comment threadtest/parallel/test-primordials-access.js Outdated

mscdex reviewed Jan 10, 2021
View reviewed changes

Comment threadtest/parallel/test-primordials-restricted-access.js Outdated

mscdex reviewed Jan 10, 2021
View reviewed changes

Comment threadlib/internal/per_context/primordials.js Outdated

devsnek requested a review from bmeck January 10, 2021 23:47

bmeck suggested changes Jan 11, 2021
View reviewed changes

bmeck approved these changes Jan 14, 2021
View reviewed changes

Comment threadtest/parallel/test-primordials-access.js Outdated

jasnell reviewed Jan 14, 2021
View reviewed changes

Comment threadlib/internal/bootstrap/pre_execution.js Outdated

jasnell approved these changes Jan 14, 2021
View reviewed changes

ljharb reviewed Jan 14, 2021
View reviewed changes

Comment threadlib/internal/bootstrap/pre_execution.js Outdated

aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 14, 2021

devsnek reviewed Jan 14, 2021
View reviewed changes

Comment threadlib/internal/bootstrap/pre_execution.js Outdated

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

aduh95 force-pushed the expose-primordials branch from a18f3ca to ba79489Compare January 15, 2021 13:55

aduh95 force-pushed the expose-primordials branch from ba79489 to b74793fCompare January 15, 2021 14:06

bmeck approved these changes Jan 15, 2021
View reviewed changes

ExE-Boss reviewed Jan 17, 2021
View reviewed changes

Comment threadlib/internal/bootstrap/loaders.js Outdated

aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 17, 2021

aduh95 force-pushed the expose-primordials branch from 2f67dae to 983e922Compare January 18, 2021 10:44

aduh95 merged commit 983e922 into nodejs:masterJan 18, 2021

aduh95 deleted the expose-primordials branch January 18, 2021 10:45

aduh95 changed the title ~~lib: add --expose-primordials flag~~lib: expose primordials objectJan 20, 2021

ruyadorno mentioned this pull request Jan 22, 2021
v15.7.0 proposal #37020
Merged

targos mentioned this pull request Jun 6, 2021
v14.17.1 release proposal #38948
Merged

fatso83 mentioned this pull request Nov 3, 2021
HTTP Date header time faking fails on Node 15.2.0 and later sinonjs/fake-timers#344
Closed

benjamingr mentioned this pull request Nov 5, 2021
lib: allow mocking primordials with --expose-internals #40733
Closed

Uh oh!

Uh oh!

Conversation

aduh95 commented Jan 10, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devsnek commented Jan 10, 2021

Uh oh!

Uh oh!

aduh95 commented Jan 10, 2021

Uh oh!

targos commented Jan 11, 2021

Uh oh!

bmeck commented Jan 11, 2021

Uh oh!

bmeck left a comment

Choose a reason for hiding this comment

Uh oh!

bmeck commented Jan 11, 2021

Uh oh!

addaleax commented Jan 11, 2021

Uh oh!

ExE-Boss commented Jan 13, 2021

Uh oh!

aduh95 commented Jan 14, 2021

Uh oh!

Uh oh!

bmeck commented Jan 14, 2021

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 14, 2021

Uh oh!

nodejs-github-bot commented Jan 14, 2021

Uh oh!

Uh oh!

aduh95 commented Jan 15, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Jan 15, 2021

Uh oh!

nodejs-github-bot commented Jan 15, 2021

Uh oh!

mscdex commented Jan 16, 2021

Uh oh!

aduh95 commented Jan 16, 2021

Uh oh!

nodejs-github-bot commented Jan 16, 2021

Uh oh!

nodejs-github-bot commented Jan 16, 2021

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 17, 2021

Uh oh!

aduh95 commented Jan 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

aduh95 commented Jan 10, 2021•
edited
Loading

aduh95 commented Jan 15, 2021•
edited
Loading