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

src: remove process.binding('config').debugOptions #25999

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Feb 8, 2019

process.binding('config').debugOptions, which contains the initial
values of parsed debugger-related CLI options, has been used for
internal testing. This patch removes them and uses internal/options
to query the values in the tests instead.

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 8, 2019
@joyeecheung
Copy link
Member Author

`process.binding('config').debugOptions`, which contains the initial
values of parsed debugger-related CLI options, has been used for
internal testing. This patch removes them and uses `internal/options`
to query the values in the tests instead.
@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

Ping @nodejs/process @addaleax can I have some review please?

@addaleax
Copy link
Member

CI is green … Should this be labelled dont-land-on-v11.x? Like, is there a chance people have been using this through process.binding('config')?

@joyeecheung
Copy link
Member Author

joyeecheung commented Feb 18, 2019

@addaleax We can label it don't land on v11 on the safe side, although the properties exposed here are not even really useful for non-test use cases as they only capture the parsed CLI options, and are not in sync with the actual state of the process. e.g. if someone wants to know about the port they are more likely to use the documented process.debugPort which also comes with a setter.

@joyeecheung
Copy link
Member Author

joyeecheung commented Feb 18, 2019

@danbev
Copy link
Contributor

danbev commented Feb 19, 2019

Landed in b200a46.

@danbev danbev closed this Feb 19, 2019
pull bot pushed a commit to shakir-abdo/node that referenced this pull request Feb 19, 2019
`process.binding('config').debugOptions`, which contains the initial
values of parsed debugger-related CLI options, has been used for
internal testing. This patch removes them and uses `internal/options`
to query the values in the tests instead.

PR-URL: nodejs#25999
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Mar 5, 2019
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.

5 participants