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.

Sign up for GitHub

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

Jump to bottom

fix: Replace nextTick by setImmediate. #360

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: Replace nextTick by setImmediate. #360

wants to merge 2 commits into from

Conversation

regseb
Copy link

@regseb regseb commented Aug 1, 2022 •
edited

Replace process.nextTick() by setImmediate() to be closer to the reality of asynchronous methods. This pull request fixes #352 testcase with yauzl.

@3cp
Copy link
Collaborator

3cp commented Aug 2, 2022

The CI shows this fix only works for Nodejs v16+.
You can try to add a condition to test against nodejs version.

@regseb
Copy link
Author

regseb commented Aug 2, 2022

I replaced the nextTick only for the promise and it seems to work on all versions of Node. Maybe the callbacks are executed directly after, while the promises are executed on the next event loop.

@3cp
Copy link
Collaborator

3cp commented Aug 3, 2022

@tschaub can you review this? I am not sure about the side effect of this change.

@3cp 3cp requested a review from tschaub August 13, 2022 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tschaub tschaub Awaiting requested review from tschaub

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Different behavior with yauzl
2 participants
@regseb @3cp