Skip to content

timers: cleanup extraneous property on Immediates#16355

Closed
targos wants to merge 2 commits intonodejs:masterfrom
targos:timers-cleanup-immediate
Closed

timers: cleanup extraneous property on Immediates#16355
targos wants to merge 2 commits intonodejs:masterfrom
targos:timers-cleanup-immediate

Conversation

@targos
Copy link
Member

@targos targos commented Oct 21, 2017

Continuation of #10205
I'd like to have this in 9.0.0.

/cc @nodejs/tsc @Fishrock123

@Fishrock123 @targos
timers: cleanup extraneous property on Immediates
eaea7a1 
This was originally changed in 6f75b66
but it appears unnecessary and benhcmark results show little difference
without the extra property.

Refs: nodejs#6436
@targos targos added semver-major PRs that contain breaking changes and should be released in the next major version. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Oct 21, 2017
@targos targos added this to the 9.0.0 milestone Oct 21, 2017
@nodejs-github-bot nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Oct 21, 2017
@targos targos mentioned this pull request Oct 21, 2017
Closed
3 tasks
@targos
Copy link
Member Author

targos commented Oct 21, 2017

quick check: https://ci.nodejs.org/job/node-test-commit-linuxone/9589/

lpinca
lpinca approved these changes Oct 21, 2017
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

SGTM

@mscdex
Copy link
Contributor

mscdex commented Oct 21, 2017

There are more instances of _callback than _onImmediate, so there would be less noise if we switched to all _callback, unless there is some other reason to keep _onImmediate instead?

@Fishrock123
Copy link
Contributor

Fishrock123 commented Oct 21, 2017 •
edited
Loading

unless there is some other reason to keep _onImmediate instead?

That private API has been there since the beginning of time. (Also it mirrors _onTimeout.)

@bnoordhuis
Copy link
Member

bnoordhuis commented Oct 21, 2017

since the beginning of time

I see what you did there.

@targos
Copy link
Member Author

targos commented Oct 21, 2017

Fixed the message test.
CI: https://ci.nodejs.org/job/node-test-pull-request/10889/

Fishrock123
Fishrock123 approved these changes Oct 21, 2017
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I was still going to rebase and merge myself, but I guess I won't complain about a rebase done for me.

CTC (at the time) already voted that the predecessor could land, fwiw.

@refack
Copy link
Contributor

refack commented Oct 22, 2017 •
edited
Loading

Question: since this is major is there any reason not to put it behind a Symbol? Or maybe even in a module internal Map?
(a Map brings this one step closer to a making immediate a standards compliant opaque identifier)

@jasnell
Copy link
Member

jasnell commented Oct 22, 2017

Just FYI... Next 9.0.0 RC will be generated on monday afternoon PDT. After that, I really don't want to land any more majors in 9.0.0.

@Fishrock123
Copy link
Contributor

Fishrock123 commented Oct 22, 2017

Question: since this is major is there any reason not to put it behind a Symbol? Or maybe even in a module internal Map?

See previous comments, the same property already has existed since 0.1.x, this just removes a recently added duplicate property.

@targos
Copy link
Member Author

targos commented Oct 23, 2017

Landed in 839faae

@targos targos closed this Oct 23, 2017
targos pushed a commit that referenced this pull request Oct 23, 2017
@Fishrock123 @targos
timers: cleanup extraneous property on Immediates
839faae 
This was originally changed in 6f75b66
but it appears unnecessary and benhcmark results show little difference
without the extra property.

Refs: #6436
PR-URL: #16355
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@targos targos deleted the timers-cleanup-immediate branch October 23, 2017 12:43
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@Fishrock123 @addaleax
timers: cleanup extraneous property on Immediates
458c55a 
This was originally changed in 6f75b66
but it appears unnecessary and benhcmark results show little difference
without the extra property.

Refs: nodejs/node#6436
PR-URL: nodejs/node#16355
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
addaleax pushed a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@Fishrock123 @addaleax
timers: cleanup extraneous property on Immediates
b91772a 
This was originally changed in 6f75b66
but it appears unnecessary and benhcmark results show little difference
without the extra property.

Refs: nodejs/node#6436
PR-URL: nodejs/node#16355
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

+3 more reviewers

@apapirovski apapirovski apapirovski approved these changes

@refack refack refack approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

semver-major PRs that contain breaking changes and should be released in the next major version. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.

Projects

None yet

Milestone

9.0.0

Development

Successfully merging this pull request may close these issues.

10 participants

@targos @mscdex @Fishrock123 @bnoordhuis @refack @jasnell @apapirovski @addaleax @lpinca @nodejs-github-bot