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

[v11.x backport] src: cleanup cli options on internalBinding('config') #26027

Closed

Conversation

ZYSzys
Copy link
Member

@ZYSzys ZYSzys commented Feb 10, 2019

Backport #25463

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v11.x labels Feb 10, 2019
src/node_config.cc Outdated Show resolved Hide resolved
Instead of using `internalBinding('config')` which should be used
to carry information about build-time options, directly pass the
run-time cli options into bootstrap/loaders.js lexically via
function arguments.

PR-URL: nodejs#25463
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Remove the following properties:

- `preserveSymlinks`
- `preserveSymlinksMain`
- `experimentalModules`
- `userLoader`
- `experimentalVMModules`
- `experimentalREPLAwait`
- `exposeInternals`

We used to use them to pass cli option values from C++ into
JS, but now the values are obtained in JS land using
`require('internal/options').getOptionValue` instead so they
are unused.

Also removes `test/parallel/test-internal-modules-expose.js`
which tests `--expose-internals`.
We already have hundreds of tests depending on `--expose-internals`,
they are more than enough to test the functionality of the flag.

PR-URL: nodejs#25463
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@targos
Copy link
Member

targos commented Feb 10, 2019

targos pushed a commit that referenced this pull request Feb 10, 2019
Instead of using `internalBinding('config')` which should be used
to carry information about build-time options, directly pass the
run-time cli options into bootstrap/loaders.js lexically via
function arguments.

PR-URL: #25463
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Backport-PR-URL: #26027
targos pushed a commit that referenced this pull request Feb 10, 2019
Remove the following properties:

- `preserveSymlinks`
- `preserveSymlinksMain`
- `experimentalModules`
- `userLoader`
- `experimentalVMModules`
- `experimentalREPLAwait`
- `exposeInternals`

We used to use them to pass cli option values from C++ into
JS, but now the values are obtained in JS land using
`require('internal/options').getOptionValue` instead so they
are unused.

Also removes `test/parallel/test-internal-modules-expose.js`
which tests `--expose-internals`.
We already have hundreds of tests depending on `--expose-internals`,
they are more than enough to test the functionality of the flag.

PR-URL: #25463
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Backport-PR-URL: #26027
@targos
Copy link
Member

targos commented Feb 10, 2019

Landed in 7565586 and d342707

@targos targos closed this Feb 10, 2019
@ZYSzys ZYSzys deleted the backport-25463-to-v11.x branch February 10, 2019 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants