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.

Sign up for GitHub

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

Jump to bottom

src: rename process._inspectorEnbale #13460

Merged
merged 1 commit into from
Jun 6, 2017
Merged

src: rename process._inspectorEnbale #13460

merged 1 commit into from
Jun 6, 2017

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jun 5, 2017

This seems like a typo. This commit changes the property to process._inspectorEnable.

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

src

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jun 5, 2017
refack
refack approved these changes Jun 5, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

🤦‍♂️ at least it so new nobody's used it yet

@refack
Copy link
Contributor

refack commented Jun 5, 2017

Any reason not to fast track this?
/cc @nodejs/diagnostics @jasnell

@refack
Copy link
Contributor

refack commented Jun 5, 2017

@cjihrig Thanks for finding this 🎁

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 5, 2017

Heh, I didn't exactly find it. #9659 (comment)

refack
refack reviewed Jun 5, 2017
View reviewed changes
src/node.cc Outdated
@@ -3404,7 +3404,7 @@ void SetupProcessObject(Environment* env,
// --inspect
if (debug_options.inspector_enabled()) {
READONLY_DONT_ENUM_PROPERTY(process,
"_inspectorEnbale", True(env->isolate()));
"_inspectorEnable", True(env->isolate()));
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe _inspectorEnabled is better?

@refack refack mentioned this pull request Jun 5, 2017
Closed
3 tasks
@refack
Copy link
Contributor

refack commented Jun 5, 2017

So @mutantcornholio the 🏆 goes to you.

@mscdex
Copy link
Contributor

mscdex commented Jun 5, 2017

Can we add a test for this?

@mscdex mscdex added inspector Issues and PRs related to the V8 inspector protocol process Issues and PRs related to the process subsystem. labels Jun 5, 2017
@refack
Copy link
Contributor

refack commented Jun 5, 2017

Can we add a test for this?

I'll do a new PR first thing tomorrow. But IMHO lack of testing should not block this.

@mcollina
Copy link
Member

mcollina commented Jun 5, 2017

@cjihrig there is a typo on the commit message and the title of the PR.

@lucamaraschi
Copy link
Contributor

@mcollina I think that was the fix of this PR ;-)

@mcollina
Copy link
Member

mcollina commented Jun 5, 2017

oooh, that was not clear.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 5, 2017

Updated to _inspectorEnabled as @refack suggested. CI: https://ci.nodejs.org/job/node-test-pull-request/8481/

@refack
Copy link
Contributor

refack commented Jun 5, 2017

I say land after 24 hours (in 10 hours 02:00 UTC) unless anyone objects
/cc @nodejs/collaborators

@sam-github
Copy link
Contributor

I thought it might be just for our internal use, to communicate from C++ init to later js, but we have no references to this property anywhere other than its definition. So, must be for external users? If so, isn't this semver-major?

@mutantcornholio
Copy link
Contributor

Well, first internal use will be here (after this PR gets merged): #9659 (comment)

@refack
Copy link
Contributor

refack commented Jun 5, 2017

I thought it might be just for our internal use, to communicate from C++ init to later js, but we have no references to this property anywhere other than its definition. So, must be for external users? If so, isn't this semver-major?

It's brand new (16689e3), and AFAIK never used. Was a bit of future proofing.
@cjihrig worth adding Ref: https://github.com/nodejs/node/pull/12949

@sam-github
Copy link
Contributor

Alternatively, if the property is unused, maybe it should be deleted, see #13228 (comment) and conversation after.

@refack
Copy link
Contributor

refack commented Jun 5, 2017

Alternatively, if the property is unused, maybe it should be deleted, see #13228 (comment) and conversation after.

IMHO it's an interesting property to have Re: #9659 and especially #13228 where inspector could be enabled without a trace in process.argv

@sam-github
Copy link
Contributor

Note that #13228 allows checking whether the inspector port is open or not with a documented API (instead of an _ prefixed and undocumented property on process), and is accurate whether the port was opened using --inspect[-brk], SIGUSR1, process._debugBegin(), inspector.open(), and whether it was closed later, then reopened, etc.

@refack
Copy link
Contributor

refack commented Jun 5, 2017 •
edited
Loading

Note that #13228 allows checking whether the inspector port is open or not with a documented API (instead of an _ prefixed and undocumented property on process), and is accurate whether the port was opened using --inspect[-brk], SIGUSR1, process._debugBegin(), inspector.open(), and whether it was closed later, then reopened, etc.

Then kill it (either in #13228 or here if you land #13228)

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 5, 2017

I'm fine with removing it here. That still leaves the semver question though.

@refack
Copy link
Contributor

refack commented Jun 5, 2017 •
edited
Loading

That still leaves the semver question though.

It's an _XX property, {enumerable: false}, undocumented, and has been in existence for < 10 days. IMHO it clearly falls within the definition of "internal". I call semver-patch.
But if the plan is to kill it, I retract the call to fast-track.

@sam-github
Copy link
Contributor

I think the policy would be to deprecate process._inspectorEnbale (note spelling) in 9.x, and delete it in 10.x. But @refack makes a compelling argument in #13460 (comment), and the CTC can do the right thing by agreement even if its not the official policy, though I think they have to formally agree that disregarding stability policy is the right thing to do here. @cjihrig would understand best the options here.

@gibfahn
Copy link
Member

gibfahn commented Jun 5, 2017

I think the policy would be to deprecate process._inspectorEnbale (note spelling) in 9.x, and delete it in 10.x.

AIUI the formal policy is that underscore properties are not subject to semver (although the CTC makes exceptions for things that people use).

@jasnell
Copy link
Member

jasnell commented Jun 5, 2017

Given that this is (a) new and (b) and obvious mistake, it's worth fixing as a semver-patch.

jasnell
jasnell approved these changes Jun 5, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

This should not need to wait the 48 hours to land

@Trott
Copy link
Member

Trott commented Jun 5, 2017 •
edited
Loading

Adding my voice to "If this requires CTC-approval to be treated as semver-patch, please record my approval".

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 5, 2017

Just to clarify, are we talking about landing this as is, or removing the property?

@Trott
Copy link
Member

Trott commented Jun 5, 2017

@cjihrig I meant either renaming or removing. I'm fine with treating either as semver-patch in this specific instance.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jun 6, 2017

CI to remove the property: https://ci.nodejs.org/job/node-test-pull-request/8497/

@cjihrig
src: remove process._inspectorEnbale
df5d8e0 
This commit removes process._inspectorEnbale which was
spelled incorrectly, and is being properly implemented
in a separate PR.

Refs: nodejs#12949
PR-URL: nodejs#13460
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luca Maraschi <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig cjihrig merged commit df5d8e0 into nodejs:master Jun 6, 2017
@cjihrig cjihrig deleted the typo branch June 6, 2017 02:18
@refack
Copy link
Contributor

refack commented Jun 6, 2017

🎉

jasnell pushed a commit that referenced this pull request Jun 7, 2017
@cjihrig @jasnell
src: remove process._inspectorEnbale
640101b 
This commit removes process._inspectorEnbale which was
spelled incorrectly, and is being properly implemented
in a separate PR.

Refs: #12949
PR-URL: #13460
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Luca Maraschi <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@lucamaraschi lucamaraschi lucamaraschi approved these changes

@jasnell jasnell jasnell approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol process Issues and PRs related to the process subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.