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

timers: cleanup extraneous property on Immediates #16355

Closed
wants to merge 2 commits into from
Closed

timers: cleanup extraneous property on Immediates #16355

wants to merge 2 commits into from

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

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

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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
@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 <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

@apapirovski apapirovski apapirovski approved these changes

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