timers: expose timer classes

[bengl]

Checklist

• make -j4 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

This allows for determining whether an arbitrary object is a timer object, using instanceof. For example, code intent on unreffing or clearing timers passed to it may want to check that the are, in fact, timers first.

Test included. Docs already covered timer classes.

[Fishrock123]

@bengl This will also need some docs in the form that timer classes cannot "just be" constructed usefully, and sub-classing is not especially useful either.

I do think it is a good idea though.

I ran into this the other day, my workaround code... :/

loggingTimer.constructor.name === 'Timeout'

[cjihrig]

The doc update sounds a bit intimidating. It might be a better idea to expose something like isTimer().

[Fishrock123]

@cjihrig That might be better.

We could probably also do some detection for Intervals that way. (Although isTimeout would still say that Intervals are Timeouts by class.)

[jasnell]

+1 to a simple isTimer() API as an alternative.

[bengl]

There are precedents for classes that are exposed and yet of little use when manually constructed or extended. For example, http.IncomingMessage.

instanceof definitely feels more natural to me when working with core APIs. I see instanceof EventEmitter a lot in the wild.

I can definitely add something like This class is intended to be instantiated in core only. It's not useful to instantiate or extend it yourself outside of core. etc. I'm also happy to go through and find other classes that don't make sense when instantiated outside of core, and document them as such, if that's helpful.

[targos]

We could create and expose a fake class and use Symbol.hasInstance to make it return true on timer objects.

[sindresorhus]

isTimer() would be best. instanceof doesn't work across realms/contexts (windows/frames in the browser), like the vm module. That's why we have Array.isArray.

[vkurchatkin]

-1. Why would would you ever need this? If you create a timer, you know it's a timer. If you don't, you shouldn't care.

[S-YOU]

may be its good to know when serializing objects, to skip?

[Fishrock123]

Sometimes you want test assertions for this. it IS useful.

[vkurchatkin]

Sometimes you want test assertions for this. it IS useful.

Can you give an example? In browser these are simply numbers and it seems to be ok

[ChALkeR]

If we export Timeout and Immediate, the next logical step would be to properly document those. Why should we export them without documenting? That makes no sense — that tells users «hey, inspect objects manually and use undocumented things».

-1.

Also, timers module is Locked, and this is an API change. This PR should be rejected per our documentation, in any form (as long as it's on the timers module), be it exposing the constructors or introducing a new function.

[ChALkeR]

Btw, work-around:

const Immediate = setImmediate(() => {}).constructor;
const Timeout = setTimeout(() => {}).constructor;

I would highly recommend against using that though, as that actually relies on implementation details and is not portable between Node.js and browsers (that is important, as the setTimeout part doesn't explicitly use any Node.js-specific API, so you might not notice that this is broken when moving code around).

[bengl]

@sindresorhus That's true for many other classes too. That sounds like a big can of worms versus just exposing these two classes with the understanding that instanceof is not going to work across contexts, which is consistent with other exposed classes throughout the API.

@vkurchatkin Objects can be passed around between modules. The types of the objects being passed in to exported functions aren't always a given.

@ChALkeR

If we export Timeout and Immediate, the next logical step would be to properly document those.

They are documented to some degree.

Also, timers module is Locked, and this is an API change.

This would be an API addition, by exposing classes whose instances we already provide to users, making it semver-minor, but yes, Locked seems stricter than semver here. I'm not sure if there's any precedent for changing a locked API in this kind of way (exposing classes that are already being used/returned). It's also worth noting that the ref() and unref() methods were added after the API was locked.

Btw, work-around

Yep! I'm doing something like this right now, and it works fine. It would be quite helpful to have them directly exposed though.

I would highly recommend against using that though, as that actually relies on implementation details and is not portable between Node.js and browsers (that is important, as the setTimeout part doesn't explicitly use any Node.js-specific API, so you might not notice that this is broken when moving code around).

Since setTimeout and setInterval already return objects with a Node.js-specific API (ref() and unref()), I'm not sure this is very important.

[vkurchatkin]

@bengl you need to know that object is a timer only if you want to cancel it. I can't imagine a function that simultaneously accepts timers and something else, and cancels a timer if it is. In practice it seems to be useful only for writing bad code

You can also thing of it as an internal detail: you get an object with ref and unref methods. It can be an instance of some constructor now, but a plain object in the future.

[ChALkeR]

@bengl

They are documented to some degree.

Well, they are documented as being internal, without any information on how to use them or manually instantiate objects of that type, which one would expect if the constructors are exported. Documentation changes would still be needed if those are exported.

This would be an API addition

Which falls under «API changes» imo, noting the full description of the stability level:

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

Between «security», «performance», «bug fixes», and «API changes», this looks much more like an API change to me.

It's also worth noting that the ref() and unref() methods were added after the API was locked.

And I view that as an issue of our process or our documentation. See #6528.
I don't think that doing that more would help, at least until we change stability levels or their descriptions.

[deian]

A a user, the is... seems less appealing since pretty much everything following this pattern (as exposed by util.is...) seems to have be deprecated. As far as I can tell Buffer.isBuffer is the only exception.

It seems like trusting JS's reflection (Array.isArray is JS, not node) vs. exposing seemingly inconsistent APIs would make our lives easier. (Would one also expose isIncomingMessage, isEventEmitter, etc.?)

[fhinkel]

Feel free to reopen this PR if you get back to working on it.

[ChALkeR]

Note that some of my comments above are not applicable now, because timers API was unlocked.