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

Fix setImmediate polyfill args #75

Merged
merged 1 commit into from
Jun 23, 2021
Merged

Fix setImmediate polyfill args #75

merged 1 commit into from
Jun 23, 2021

Conversation

voces
Copy link
Contributor

@voces voces commented Jun 12, 2021

This fixes the setImmediate polyfill to accept rest-style args instead of an array to match Node.js.

// Actual node.js type signature
type setImmediate = <Args extends any[]>(fn: (...args: Args) => void, ...args: Args);

// Previous esm.sh type signature:
type setImmediate = <Args extends any[]>(fn: (...args: Args) => void, args: Args);

The previous behavior would result in setImmediate performing differently. If no arguments were passed or the first argument was not iterable, an exception would be thrown. If the first argument was iterable, it'd be passed as a spread result. I created an example npm module and ran it in deno repl as an example:

> const examples = await import("http://esm.sh/esm-sh-set-immediate-test").then(i => i.default);
> examples.withArray()
> foo bar // expected args [["foo", "bar"]], got ["foo", "bar"]
> examples.withArgs();
> f o o // expected args ["foo", "bar"], got ["f", "o", "o"]
> examples.withNone();
// expected args [], got below exception
Uncaught TypeError: args is not iterable (cannot read property undefined)
    at __setImmediate$ (https://cdn.esm.sh/v41/esm-sh-set-immediate-test@1.0.0/deno/esm-sh-set-immediate-test.js:2:37)
    at Object.withNone (https://cdn.esm.sh/v41/esm-sh-set-immediate-test@1.0.0/deno/esm-sh-set-immediate-test.js:2:833)
    at <anonymous>:2:10

@ije
Copy link
Member

ije commented Jun 23, 2021

lgtm, thanks

@ije ije merged commit 8a05613 into esm-dev:master Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants