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

fastTimeout tests might actually run setImmediate under the hood #2

Open
jgonggrijp opened this issue Mar 12, 2024 · 0 comments · May be fixed by #5
Open

fastTimeout tests might actually run setImmediate under the hood #2

jgonggrijp opened this issue Mar 12, 2024 · 0 comments · May be fixed by #5
Labels
good first issue Good for newcomers question Further information is requested

Comments

@jgonggrijp
Copy link
Contributor

While debugging, I had a look at the bundle that Rollup generates inside the Karma test runner. It appears to wrap the test module in some code that injects the setImmediate function from Node.js. This is probably done by @rollup/plugin-node-resolve. If this is indeed the case, then the tests are not actually checking whether fastTimeout is implemented correctly, but whether setImmediate is a good alternative. This could be determined by running the tests in debug mode and setting a breakpoint in the fastTimeout function. If the breakpoint is never encountered, we are testing setImmediate instead.

A possible fix is to sophisticate the export default statement in src/fastTimeout.js. Right now, it prioritizes setImmediate if present. This could be changed to prioritize our own fastTimeout if postMessage is present. This might be a good idea, anyway. There should probably also be a third alternative in case neither is present; _.defer would work for that scenario, which is equivalent to _.partial(setTimeout, _, 0).

@jgonggrijp jgonggrijp added good first issue Good for newcomers question Further information is requested labels Mar 12, 2024
@jgonggrijp jgonggrijp linked a pull request Dec 10, 2024 that will close this issue
@jgonggrijp jgonggrijp linked a pull request Dec 10, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant