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.

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

timers: unlock the timers API #11580

Closed
wants to merge 1 commit into from
Closed

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

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.

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

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

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.

@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
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

@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
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
MylesBorins pushed a commit that referenced this pull request Apr 17, 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.

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
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
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 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.

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
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
Development

Successfully merging this pull request may close these issues.

10 participants