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,lib: prefer internal/options over process._foo #25063

Closed
wants to merge 1 commit into from

Conversation

@addaleax
Copy link
Member

commented Dec 15, 2018

This addresses a couple TODO comments and allows us
to remove a number of underscored properties from process
(in a semver-major follow-up).

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

@addaleax addaleax requested a review from joyeecheung Dec 15, 2018

@@ -104,7 +105,7 @@ function startup() {
NativeModule.require('internal/inspector_async_hook').setup();
}

const { getOptionValue } = NativeModule.require('internal/options');
getOptionValue = NativeModule.require('internal/options').getOptionValue;

This comment has been minimized.

Copy link
@mscdex

mscdex Dec 15, 2018

Contributor

We could keep the same extraction method as before with:

({ getOptionValue } = NativeModule.require('internal/options'));

This comment has been minimized.

Copy link
@addaleax

addaleax Dec 15, 2018

Author Member

@mscdex I thought about it but thought this was maybe easier to read without the parenthesis? If you’d prefer this, I’d be totally okay with that too?

This comment has been minimized.

Copy link
@mscdex

mscdex Dec 15, 2018

Contributor

I don't mind either way, was just throwing that out there.

src,lib: prefer internal/options over process._foo
This addresses a couple `TODO` comments and allows us
to remove a number of underscored properties from `process`
(in a semver-major follow-up).

@addaleax addaleax force-pushed the addaleax:less-process-dot-something branch from 174cee9 to 8f105f8 Dec 15, 2018

@joyeecheung
Copy link
Member

left a comment

LGTM. Thanks for picking this up! I tried to take a stab at this a while back but it was more tricky than I thought to eliminate some of these from JS land (tests were failing/hanging for legit reasons which I was not able to figure out), but it's good to get rid of them incrementally 👍 And looking forward to the followup!

@danbev

This comment has been minimized.

@danbev

This comment has been minimized.

Copy link
Member

commented Dec 21, 2018

Re-run of failing node-test-commit-aix ✔️
Re-run of failing node-test-commit-smartos ✔️

@danbev

This comment has been minimized.

Copy link
Member

commented Dec 21, 2018

Landed in 8b57208.

@danbev danbev closed this Dec 21, 2018

danbev added a commit that referenced this pull request Dec 21, 2018
src,lib: prefer internal/options over process._foo
This addresses a couple `TODO` comments and allows us
to remove a number of underscored properties from `process`
(in a semver-major follow-up).

PR-URL: #25063
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

This comment has been minimized.

Copy link
Member

commented Dec 25, 2018

This doesn't land cleanly on v11.x, should it be backported?

@targos targos added this to Backport requested in v11.x Dec 28, 2018

@addaleax addaleax deleted the addaleax:less-process-dot-something branch Jan 9, 2019

@BridgeAR BridgeAR moved this from Backport requested to Backported in v11.x Jan 10, 2019

@BridgeAR BridgeAR moved this from Backported to Backport requested in v11.x Jan 10, 2019

addaleax added a commit that referenced this pull request Jan 14, 2019
src,lib: prefer internal/options over process._foo
This addresses a couple `TODO` comments and allows us
to remove a number of underscored properties from `process`
(in a semver-major follow-up).

PR-URL: #25063
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

Applies cleanly now.

refack added a commit to refack/node that referenced this pull request Jan 14, 2019
src,lib: prefer internal/options over process._foo
This addresses a couple `TODO` comments and allows us
to remove a number of underscored properties from `process`
(in a semver-major follow-up).

PR-URL: nodejs#25063
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@BridgeAR BridgeAR moved this from Backport requested to Backported in v11.x Jan 14, 2019

@BridgeAR BridgeAR referenced this pull request Jan 16, 2019
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
src,lib: prefer internal/options over process._foo
This addresses a couple `TODO` comments and allows us
to remove a number of underscored properties from `process`
(in a semver-major follow-up).

PR-URL: nodejs#25063
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins referenced this pull request Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
10 participants
You can’t perform that action at this time.