Add further stronger typing and depend on globalThis

[43081j]

Purpose (TL;DR) - mandatory

Continued from #370.

I've tried to do the following:

• Depend on globalThis, such that things like setTimeout are strongly typed as being identical to typeof globalThis.setTimeout - this should theoretically mean you get the right types depending on the environment you're in (node, browser)
• Define node-specific functions ourselves to avoid pulling node types in
• Define what timers
• Add jsdoc to withGlobal but not a return type (so typescript will infer it)

If someone can double check that depending on the global types makes sense and will work fine, that'd be much appreciated. It seems to for me

cc @fatso83

[codecov[bot]]

Codecov Report

Merging #371 (3caaa2d) into master (5139ea9) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #371   +/-   ##
=======================================
  Coverage   93.49%   93.49%           
=======================================
  Files           1        1           
  Lines         553      553           
=======================================
  Hits          517      517           
  Misses         36       36           

Flag	Coverage Δ
unit	93.49% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/fake-timers-src.js	93.49% <ø> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5139ea9...3caaa2d. Read the comment docs.

[fatso83]

Thanks for this! I see there is a big discussion leading up to this on DT and I think we are well aware that the types we ship are not as up to snuff as the ones on DT, although we hope that might change in time :)

If someone can double check that depending on the global types makes sense and will work fine

TBH, I am not sure how to verify that, given that I don't have any typescript consumers at the moment, but a manual inspection of this seems to imply these changes are beneficial ...

[43081j]

fwiw i ran the build and manually checked the produced types by eye, which is how i got to these changes.

tomorrow if i get chance, i will run the DT tests against them and that should be enough to verify if they are fine IMO.

its also a major version bump over on DT so if we accidentally weaken some types, thats ok, ill fix them up in separate PRs.

[43081j]

@stof @fatso83 have pushed the fixes to those constructor types, mind taking another look?

[43081j]

one thing to note here that i need double checking:

let num: number = clock.setTimeout(fn, 1000);

// in a browser, this is a `number`
// in node, it is a `NodeTimer`, an object
// this is expected and fine.

// However, the following won't work:
num = clock.setImmediate(fn);

// This is a `NodeTimer`, always, as setImmediate doesn't exist in browsers.
// It can't be assigned to a `number`

This shouldn't ever happen but we can't really type it easily through JSDoc.

In reality, the Clock type would never have a setImmediate in browsers but would always have one in node. Making the above code impossible in browsers.

Because its not typed that way, there's nothing stopping a browser consumer trying to use setImmediate, but it wouldn't really exist

You kind of want to do:

type Clock = {
  // ...
  setImmediate: isNode ? (typeof globalThis.setImmediate) : never;
}

but it starts getting complex so maybe its safe to just assume people will understand not to expect it to work in browsers?

[fatso83]

the Clock type would never have a setImmediate in browsers

Actually, that is not entirely true. I have used setImmediate lots of times: it was in IE10, IE11 and Edge 12-18 and polyfills were available for all the rest. Now that the world, including MS has essentially gone mono-browser (via Chrome) it is practically true for evergreen browsers 😄

TypeScript does support Conditional Typing, so it's possible to rectify this with a 10/10 solution at some time, but I think your solution is good enough to merge now.

[43081j]

@fatso83 could you possibly publish a new version with this change in? So we can unblock the last change in the DT repo