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: unlock the timers API #11580

Closed
wants to merge 1 commit into from
Closed

timers: unlock the timers API #11580

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 27, 2017

Change the Stability Index on timers from Locked to Stable.

Note that this is not intended to encourage changes to the timers API,
but to allow it when its useful for Node.js (as has happened in
violation of the documented stability level), and possibly to simplify
the stability levels by removing Locked altogether.

Refs: #11200

@nodejs/ctc

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

timers

@Trott
timers: unlock the timers API
64f26e0 
Change the Stability Index on timers from Locked to Stable.

Note that this is not intended to encourage changes to the timers API,
but to allow it when its useful for Node.js (as has happened in
violation of the documented stability level), and possibly to simplify
the stability levels by removing Locked altogether.

Refs: nodejs#11200
@Trott Trott added doc Issues and PRs related to the documentations. notable-change PRs with changes that should be highlighted in changelogs. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Feb 27, 2017
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Feb 27, 2017
@Fishrock123
Copy link
Contributor

+1 if we are removing locked, -1 otherwise? I can't forsee a good reason to do this and there has not been any significant requests, language features or otherwise, to do so?

@gibfahn
Copy link
Member

gibfahn commented Feb 27, 2017

+1 if we are removing locked, -1 otherwise?

I assume the easiest way to remove Locked would be to first remove everything that's currently marked as Locked (i.e. a case-by-case analysis of whether there's a good reason for each module to be Locked rather than Stable). If we've done that then it's an easy decision to remove the category entirely.

I'd say in this case that having it be Locked doesn't add anything, so +1 as a step towards removing Locked.

ChALkeR
ChALkeR approved these changes Feb 27, 2017
View reviewed changes
Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

+1

@jasnell
Copy link
Member

jasnell commented Feb 27, 2017

I would love to see us simplify the stability matrix by dropping locked.

@Trott
Copy link
Member Author

Trott commented Feb 27, 2017

+1 if we are removing locked

That's the plan...

@Fishrock123
Copy link
Contributor

Fishrock123 commented Feb 27, 2017 •
edited
Loading

I'd say in this case that having it be Locked doesn't add anything, so +1 as a step towards removing Locked.

Not sure I agree. The intention here (as far as I can tell) was to pretty much never add public APIs to timers. I suppose that is to say, we follow browsers quite close for this module and since they don't have things besides setTimeout() and setInterval() we avoid having anything else too. (There is of course the exception for setImmediate() coming from the now years-old change to process.nextTick().

@gibfahn
Copy link
Member

gibfahn commented Feb 27, 2017 •
edited
Loading

The intention here was to pretty much never add public APIs to timers

I get the intention, but for me the difference between Locked and Stable is the pretty much in that sentence. Given that changes have and will be made (possibly in the future due to changes in ecma262), as detailed in #6528 and #11200, it doesn't seem correct to call these modules Locked.

For example, if Timers is locked, then we shouldn't really accept #11384 right? The locked definition doesn't say no new APIs, it says no changes to APIs.

Stability: 3 - Locked
Only bug fixes, security fixes, and performance improvements will be accepted.
Please do not suggest API changes in this area; they will be refused.

I think we should be resistant to (or very careful about) adding new public APIs to any of the Stable modules without clear justification. I just don't see what Locked adds here if it's not really Locked.

Stability: 2 - Stable 
The API has proven satisfactory. Compatibility with the npm ecosystem 
is a high priority, and will not be broken unless absolutely necessary.

This was referenced Feb 27, 2017
Closed
Closed
@ChALkeR
Copy link
Member

ChALkeR commented Mar 1, 2017

/cc @nodejs/ctc for additional review, as this might be controversial (I'm +1 though).

Trott added a commit to Trott/io.js that referenced this pull request Mar 2, 2017
@Trott
timers: unlock the timers API
c76f737 
Change the Stability Index on timers from Locked to Stable.

Note that this is not intended to encourage changes to the timers API,
but to allow it when its useful for Node.js (as has happened in
violation of the documented stability level), and possibly to simplify
the stability levels by removing Locked altogether.

PR-URL: nodejs#11580
Ref: nodejs#11200
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@Trott
Copy link
Member Author

Trott commented Mar 2, 2017

Landed in c76f737

@Trott Trott closed this Mar 2, 2017
@ChALkeR ChALkeR mentioned this pull request Mar 2, 2017
Closed
@Fishrock123
Copy link
Contributor

I'm still -1 on this unless we are removing Locked entirely and that is not clear to me yet. 😞

@ChALkeR
Copy link
Member

ChALkeR commented Mar 3, 2017

@Fishrock123 #11661 removes Locked entirely and has 7 LGTMs and no objections atm.

addaleax pushed a commit that referenced this pull request Mar 5, 2017
@Trott @addaleax
timers: unlock the timers API
163d2d1 
Change the Stability Index on timers from Locked to Stable.

Note that this is not intended to encourage changes to the timers API,
but to allow it when its useful for Node.js (as has happened in
violation of the documented stability level), and possibly to simplify
the stability levels by removing Locked altogether.

PR-URL: #11580
Ref: #11200
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@Trott Trott removed the ctc-review label Mar 6, 2017
@evanlucas evanlucas mentioned this pull request Mar 8, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
@Trott @MylesBorins
timers: unlock the timers API
854b3ea 
Change the Stability Index on timers from Locked to Stable.

Note that this is not intended to encourage changes to the timers API,
but to allow it when its useful for Node.js (as has happened in
violation of the documented stability level), and possibly to simplify
the stability levels by removing Locked altogether.

PR-URL: #11580
Ref: #11200
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@Trott @MylesBorins
timers: unlock the timers API
39f7aaa 
Change the Stability Index on timers from Locked to Stable.

Note that this is not intended to encourage changes to the timers API,
but to allow it when its useful for Node.js (as has happened in
violation of the documented stability level), and possibly to simplify
the stability levels by removing Locked altogether.

PR-URL: #11580
Ref: #11200
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@Trott @andrew749
timers: unlock the timers API
ccff08a 
Change the Stability Index on timers from Locked to Stable.

Note that this is not intended to encourage changes to the timers API,
but to allow it when its useful for Node.js (as has happened in
violation of the documented stability level), and possibly to simplify
the stability levels by removing Locked altogether.

PR-URL: nodejs/node#11580
Ref: nodejs/node#11200
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@Trott Trott deleted the timer-stable branch January 13, 2022 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

@ChALkeR ChALkeR ChALkeR approved these changes

@sam-github sam-github sam-github approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. notable-change PRs with changes that should be highlighted in changelogs. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@Trott @Fishrock123 @gibfahn @jasnell @ChALkeR @sam-github @targos @cjihrig @MylesBorins @nodejs-github-bot