src: rename process._inspectorEnbale

[cjihrig]

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

[refack]

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

[refack]

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

[refack]

@cjihrig Thanks for finding this 🎁

[cjihrig]

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

[refack]

maybe _inspectorEnabled is better?

[refack]

So @mutantcornholio the 🏆 goes to you.

[mscdex]

Can we add a test for this?

[refack]

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]

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

[lucamaraschi]

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

[mcollina]

oooh, that was not clear.

[cjihrig]

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

[refack]

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

[sam-github]

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]

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

[refack]

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]

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

[refack]

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]

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]

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]

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

[refack]

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]

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]

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]

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

[jasnell]

This should not need to wait the 48 hours to land

[Trott]

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

[cjihrig]

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

[Trott]

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

[cjihrig]

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

[refack]

🎉