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

lib: expose primordials object #36872

Merged
merged 1 commit into from Jan 18, 2021
Merged

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 10, 2021

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)

@nodejs-github-bot 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
@devsnek
Copy link
Member

devsnek commented Jan 10, 2021

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 devsnek requested a review from bmeck January 10, 2021 23:47
@aduh95
Copy link
Contributor Author

aduh95 commented Jan 10, 2021

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
Copy link
Member

targos commented Jan 11, 2021

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
Copy link
Member

bmeck commented Jan 11, 2021

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.

Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

bmeck commented Jan 11, 2021

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
Copy link
Member

@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
Copy link
Contributor

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
Copy link
Contributor Author

aduh95 commented Jan 14, 2021

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.

test/parallel/test-primordials-access.js Outdated Show resolved Hide resolved
@bmeck
Copy link
Member

bmeck commented Jan 14, 2021

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
Copy link
Collaborator

@aduh95 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
@nodejs-github-bot
Copy link
Collaborator

@aduh95 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
Copy link
Contributor Author

aduh95 commented Jan 15, 2021

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
Copy link
Contributor Author

aduh95 commented Jan 15, 2021

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
Copy link
Collaborator

@mscdex
Copy link
Contributor

mscdex commented Jan 16, 2021

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

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 16, 2021

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
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 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
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
Copy link
Contributor Author

aduh95 commented Jan 18, 2021

Landed in 983e922

@aduh95 aduh95 merged commit 983e922 into nodejs:master Jan 18, 2021
@aduh95 aduh95 deleted the expose-primordials branch January 18, 2021 10:45
@aduh95 aduh95 changed the title lib: add --expose-primordials flag lib: expose primordials object Jan 20, 2021
ruyadorno pushed a commit that referenced this pull request Jan 22, 2021
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>
@ruyadorno ruyadorno mentioned this pull request Jan 22, 2021
targos pushed a commit that referenced this pull request May 25, 2021
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>
targos pushed a commit that referenced this pull request May 30, 2021
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>
targos pushed a commit that referenced this pull request Jun 5, 2021
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>
targos pushed a commit that referenced this pull request Jun 11, 2021
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants