timers: cleanup extraneous property on Immediates

[Fishrock123]

Checklist

• make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

timers

Description of change

Cleans up Immediate to just always use _onImmediate for storing the callback, rather than two properties.

This was originally changed in 6f75b66
but it appears unnecessary and benchmark results show little difference
without the extra property.

Refs: #6436

Some benchmark numbers below, seems like gains from having two properties were insignificant if at all.

                                                    improvement significant     p.value
 timers/immediate.js type="breadth" thousands=2000       2.03 %           * 0.016279986
 timers/immediate.js type="breadth1" thousands=2000      0.31 %             0.696283905
 timers/immediate.js type="breadth4" thousands=2000      0.65 %             0.500713916
 timers/immediate.js type="clear" thousands=2000         2.99 %           * 0.030440595
 timers/immediate.js type="depth" thousands=2000         0.23 %             0.808941896
 timers/immediate.js type="depth1" thousands=2000       -2.23 %          ** 0.002574578

(This patch was made live during https://www.twitch.tv/nodesource/v/106354366 if you'd like to see me working on this in retrospect. :P)

CTC vote tally

In favor: 9 (@Fishrock123 @ofrobots @targos @cjihrig @evanlucas @addaleax @indutny)
Opposed: 1 (@misterdjules)
Abstain: 5 (@Trott @jasnell @MylesBorins @mhdawson @thefourtheye )

Not yet voted: 5 (@bnoordhuis @ChALkeR @chrisdickinson @rvagg @shigeki)

With 5 abstentions, 8 votes are needed to pass. It has 9 in favor. So this can land.

[Fishrock123]

cc @andrasq, @AndreasMadsen, @misterdjules

[AndreasMadsen]

We should really get nodejs/benchmarking#58 working, it is highly unlikely that these results are false positives just because of randomness. Given that we should expect some or at least no performance improvement, the most likely explanation is that something else was interfering while the benchmark was running.

[Fishrock123]

@AndreasMadsen to be fair, I made this while livestreaming lol, so yes...(https://www.twitch.tv/nodesource/v/106354366, which I've now linked to in the OP.)

[Trott]

If we're erring on the side of caution, this is semver-major because _callback is exposed to user land and after this change, it won't be...

[Fishrock123]

@Trott Heh, it was added recently and is a duplicate of an existing property (but kinda not in some cases).

That is to say: it's potentially unreliable anyways and I'm willing to take the heat for it if anyone actually complains.
¯\_(ツ)_/¯

[jasnell]

We've had things we thought shouldn't be a problem come back and bite us. I'd rather be more cautious. Perhaps land this as a semver-major and make _callback a deprecated alias for at least one major cycle? Otherwise the change LGTM

[Fishrock123]

Ok seriously, how is this so debatable when #8661 landed a while ago, removed _repeat which was in there for years and even I hand concerns, yet nothing broke?

This has been in here like 2-3 months, and doesn't even work how you'd expect. Lets remove it. Tagging ctc-agenda.

[jasnell]

Just wanting/trying to be more cautious going forward.

…

On Fri, Dec 23, 2016 at 9:20 AM Jeremiah Senkpiel ***@***.***> wrote: Ok seriously, how is *this* *so debatable* when #8661 <#8661> landed a while ago, removed *_repeat* which was in there for *years* and even I hand concerns, yet nothing broke? This has been in here like 2-3 months. Lets remove it. Tagging ctc-agenda. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#10205 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa2edCHrOrGB_UpPOX-Mebs5wuhvFTTks5rLALqgaJpZM4LJVxC> .

[Trott]

IMO, the larger issue that the CTC needs to make a decision on is:

Is semver-major merely a simple judgment that "we don't expect this to break anything in userland"? Or is the removal of any exposed property whatsoever semver-major always.

I suspect that the community would expect us to take that second approach (or something close to it--maybe an exception for accidentally exposed properties that are only in one released version of Node.js or something like that).

@Fishrock123 I sympathize with your frustration. However, I think there's a larger principle at stake here. And I think the larger issue is something the project should make an intentional decision about rather than relying on past practice as a de facto policy.

[ljharb]

It's worth noting that semver-ness is typically determined not by actual usage or breakage, but solely by theoretical breakage. If it might break some code out there, even if it's one usage inside a private codebase inside an enterprise maintained by a developer who's happy to fix it, then it's still semver-major (as frustrating as that is for me, too, when I have a change I want to land without a major bump)

[Fishrock123]

I'm still going to hold that we've made more impactful changes within this major version and we should still do this one too before people mistakenly rely on it.

[gibfahn]

Following on from @sam-github's comment here, is the public API only what's documented in https://nodejs.org/dist/latest-v6.x/docs/api/documentation.html? If so it's technically not semver-major right?

[ljharb]

imo anything reachable is part of the public API, and documentation can only opt things in to the API but not opt things out.

[gibfahn]

So according to www.semver.org you have to define a public API.

Software using Semantic Versioning MUST declare a public API. This API could be declared in the code itself or exist strictly in documentation. However it is done, it should be precise and comprehensive.

Do we declare anywhere that the public API is the code or the docs? If so then (again technically) that's the definition. If you include anything reachable, then if you're monkey-patching the code and using node --expose-internals everything is semver-major.

[andrasq]

Reachability is not really helpful here, since everything is reachable in javascript.
Languages that use convention-based public/private separation often use _ (underscore)
as prefix on the property name to indicate that it's not public.

[sam-github]

@ljharb "anything reachable is part of the public API" seems to imply that there is a private API... but how is it possible to have an unreachable private API in js? I guess we could start using unconventional mechanisms of object creation that don't ever use this, and access all methods and would-be properties via a private closure, but that is an extreme gymnastic exercise, possibly with severe performance impacts, just to assert that Node has the right to a private API and the right to have implementation details.

By such a rule, even the addition of properties to an object is semver-major, since if node starts to use a property name that someone is relying on not-existing their code may break.

We can just bump the semver-major continuously almost every time we write some code, but I don't think that's reasonable, its unrealistically constraining.

We actually have the right to define what Node considers its "API". Maybe its time we did. We could document and define every _ property as private, for example, explicitly stating that code depending on them is invalid, and can be broken at will in semver-patch updates. That definition itself would be semver-major.

[Fishrock123]

imo anything reachable is part of the public AP

You can reach pretty much anything. I'm not kidding, it is possible to re-execute node in such a way as to access setup internals. (Please for the love of anything do not do that.)

As such, this idea isn't really useful.

[jasnell]

@nodejs/ctc ... this was discussed on today's call but we were unable to reach consensus. A call for vote was suggested. Please take the time to weigh in on this PR and we'll finalize the vote next week.

For my part, my vote is -0. Since this property landed in at least one LTS stream, and because I'd like to see us being more careful moving forward with potentially breaking changes, I'd prefer if this went through a proper deprecation cycle. That said, the likelihood of actually breaking someone is exceedingly low in this case. Therefore, I'm not going to object to landing.

[Trott]

Abstaining. I'm fine with whatever as long as we improve policy so we can have fewer of these discussions in the future. :-D

[Trott]

@jasnell -0 is not a value we can use. You can vote against it or you can abstain, but we don't have a mechanism for weighing something that has elements of both, so you have to make a choice.

[jasnell]

I've been using -1/-0/+0/+1 for many years and I'm quite unlikely to break the habit any time soon. Interpret it as an abstain with a preference towards a proper deprecation cycle.

[misterdjules]

-1 for merging this without a semver-major bump. @ljharb's words earlier in this thread summarize my opinion very well:

It's worth noting that semver-ness is typically determined not by actual usage or breakage, but solely by theoretical breakage. If it might break some code out there, even if it's one usage inside a private codebase inside an enterprise maintained by a developer who's happy to fix it, then it's still semver-major (as frustrating as that is for me, too, when I have a change I want to land without a major bump)

[sam-github]

In the absence of a policy, looking at this PR standalone, I am +1 for the cleanup.

It's worth noting that semver-ness is typically determined not by actual usage or breakage, but solely by theoretical breakage.

semverness is typically defined in reference to an API, see semver.org, not to implementation detail. This is admittedly somewhat awkward with javascript. I'd hope we would take this moment to define/document what our API is. If its "anything a user's code could possibly rely on" we will have to consider the semverness of bug fixes, since one user's code can rely on behaviour that another user considers a bug.

[ofrobots]

+1. This is not part of the public API and even then it has been around for only a couple months. Leaving it in for longer would be more harmful.

[MylesBorins]

+0. I'm going to abstain

[targos]

+1

[cjihrig]

+1 since it is not official API.

[addaleax]

+1

[evanlucas]

+1

[Trott]

Depending how they vote or if they abstain, we still need at least three more CTC folks to weigh in to get resolution on this. Paging @bnoordhuis @ChALkeR @chrisdickinson @indutny @mhdawson @mscdex @rvagg @shigeki @thefourtheye @trevnorris

[indutny]

👍 internal undocumented property can go away.

[mhdawson]

I'm going to abstain.

[thefourtheye]

Same as @jasnell's, -0, with the same interpretation.

[trevnorris]

+1 for removing

[mscdex]

+1

[jasnell]

CTC vote passes on this item. It is clear to land.

[ChALkeR]

It looks like I missed the vote here and got late, sorry.

Is the vote here for merging this in without a semver-major bump?
Seems like that, given how @misterdjules vote was interpreted at the voting table.

If so, I'm -1 on this, this has to get in as a semver-major imo.

---

It doesn't look like my vote will affect anything, though, so you can ignore me, just making things clear =).

[jasnell]

Ping @Fishrock123 ... any updates on this one?

[fhinkel]

There hasn't been any activity here. I'm closing this. Feel free to reopen if I closed this in error.

[Fishrock123]

should still be merged, don't care by who

[BridgeAR]

@Fishrock123 since you reopened this, would you be so kind and rebase?

[targos]

#16355

[targos]

Landed in 839faae