sinonjs / fake-timers

Fake setTimeout and friends (collectively known as "timers"). Useful in your JavaScript tests. Extracted from Sinon.JS
BSD 3-Clause "New" or "Revised" License
797 stars 104 forks source link

Publish .d.ts files #345

Closed mroderick closed 3 years ago

mroderick commented 3 years ago

In order to improve the experience of TypeScript users, we are compiling .d.ts files from the JSDoc and distributing them with the package

This PR drops the bundled version, as we believe that to be entirely unnecessary.

Background

Solution

How to verify

  1. Check out this branch
  2. npm ci
  3. npm run build
  4. Observe that there are .d.ts files in the types/ folder
  5. npm pack
  6. Observe that the .d.ts files are included in the package
  7. npm link
  8. In a sinon checkout:
    1. npm link @sinonjs/fake-timers
    2. npm test
    3. Observe that tests still pass
codecov[bot] commented 3 years ago

Codecov Report

Merging #345 (63fc865) into master (d07b735) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #345   +/-   ##
=======================================
  Coverage   93.15%   93.15%           
=======================================
  Files           1        1           
  Lines         555      555           
=======================================
  Hits          517      517           
  Misses         38       38           
Flag Coverage Δ
unit 93.15% <ø> (ø)

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


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 d07b735...63fc865. Read the comment docs.

SimenB commented 3 years ago

This PR drops the bundled version, as we believe that to be entirely unnecessary.

Probably a breaking change (I assume it'll break CDN scripts etc).

Could you post a sample of how the generated .d.ts file looks?

mroderick commented 3 years ago

Probably a breaking change (I assume it'll break CDN scripts etc).

I'm not sure I follow. Could you elaborate?

mroderick commented 3 years ago

Could you post a sample of how the generated .d.ts file looks?

export namespace timers {
    const setTimeout: any;
    const clearTimeout: any;
    const setInterval: any;
    const clearInterval: any;
    const Date: any;
}
/**
 * @param start {Date|number} the system time - non-integer values are floored
 * @param loopLimit {number}  maximum number of timers that will be run when calling runAll()
 */
export function createClock(start: Date | number, loopLimit: number): {
    now: number;
    timeouts: {};
    Date: any;
    loopLimit: number;
};
/**
 * @param config {Object} optional config
 * @param config.now {number|Date}  a number (in milliseconds) or a Date object (default epoch)
 * @param config.toFake {string[]} names of the methods that should be faked.
 * @param config.loopLimit {number} the maximum number of timers that will be run when calling runAll()
 * @param config.shouldAdvanceTime {Boolean} tells FakeTimers to increment mocked time automatically (default false)
 * @param config.advanceTimeDelta {Number} increment mocked time every <<advanceTimeDelta>> ms (default: 20ms)
 */
export function install(config: {
    now: number | Date;
    toFake: string[];
    loopLimit: number;
    shouldAdvanceTime: boolean;
    advanceTimeDelta: number;
}, ...args: any[]): {
    now: number;
    timeouts: {};
    Date: any;
    loopLimit: number;
};
export function withGlobal(_global: any): {
    timers: {
        setTimeout: any;
        clearTimeout: any;
        setInterval: any;
        clearInterval: any;
        Date: any;
    };
    createClock: (start: Date | number, loopLimit: number) => {
        now: number;
        timeouts: {};
        Date: any;
        loopLimit: number;
    };
    install: (config: {
        now: number | Date;
        toFake: string[];
        loopLimit: number;
        shouldAdvanceTime: boolean;
        advanceTimeDelta: number;
    }, ...args: any[]) => {
        now: number;
        timeouts: {};
        Date: any;
        loopLimit: number;
    };
    withGlobal: typeof withGlobal;
};
SimenB commented 3 years ago

Probably a breaking change (I assume it'll break CDN scripts etc).

I'm not sure I follow. Could you elaborate?

People dropping in a script tag with https://unpkg.com/@sinonjs/fake-timers@6.0.1/fake-timers.js

(to be clear, I don't think this is a super important use case, just pointing out that it is a use case, and I'd be careful about breaking it without a major version)

mroderick commented 3 years ago

Probably a breaking change (I assume it'll break CDN scripts etc).

I'm not sure I follow. Could you elaborate?

People dropping in a script tag with https://unpkg.com/@sinonjs/fake-timers@6.0.1/fake-timers.js

(to be clear, I don't think this is a super important use case, just pointing out that it is a use case, and I'd be careful about breaking it without a major version)

That's fair. I've changed the label, so it'll get released properly

mroderick commented 3 years ago

This has been published as @sinonjs/fake-timers@7.0.0

SimenB commented 3 years ago

Trying to upgrade in Jest, I find I'm missing some of the helper types from DT. For reference: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/a896e8760e352dccfd90d1c5816c9cbd4088112f/types/sinonjs__fake-timers/index.d.ts

Specifically I'm missing https://github.com/DefinitelyTyped/DefinitelyTyped/blob/a896e8760e352dccfd90d1c5816c9cbd4088112f/types/sinonjs__fake-timers/index.d.ts#L370-L375 & https://github.com/DefinitelyTyped/DefinitelyTyped/blob/a896e8760e352dccfd90d1c5816c9cbd4088112f/types/sinonjs__fake-timers/index.d.ts#L312-L315

Not sure how to best handle it

  1. somehow add the types/interfaces
  2. manually maintain types def
  3. just rewrite in TypeScript

thoughts?

For reference, Jest usage: https://github.com/facebook/jest/blob/f0dc9932cba828a1c51ef842c00c1ca51b7f2796/packages/jest-fake-timers/src/modernFakeTimers.ts

SimenB commented 3 years ago

More generally, all the methods on Clock are missing from the generated types (setSystemTime, countTimers, runToLast etc). While I realize we can probably somehow add that via JSDoc, it might be worth it to just have the source in TS?

mroderick commented 3 years ago

it might be worth it to just have the source in TS?

No thanks

mroderick commented 3 years ago

More generally, all the methods on Clock are missing from the generated types (setSystemTime, countTimers, runToLast etc). While I realize we can probably somehow add that via JSDoc

Indeed. The documentation coverage in these libraries is sparing at best. I thought that if we show the TS community some love, that some of them would contribute back to the Sinon projects in ways that benefits everyone i.e. JSDoc

fatso83 commented 3 years ago

How does the mish-mash of TS files from the source package and stuff from DefinitelyTyped co-exist? Who will override and win the battle? Any clue, @SimenB .

SimenB commented 3 years ago

I believe the types shipped with a package will win (as they'll be found without trying to resolve @types/x), but I haven't verified