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

refactor: switch from process.nextTick() to queueMicrotask() #941

Merged
merged 7 commits into from
Oct 7, 2023

Conversation

SukkaW
Copy link
Contributor

@SukkaW SukkaW commented Aug 17, 2023

The PR closes #934.

The PR does the following things:

  • Add queueMicrotask ponyfill
    • The ponyfill will prefer native queueMicrotask when available, and will fallback to Promise.resolve().then otherwise.
    • The unit test cases for the ponyfill have also been added
  • Replace all process.nextTick with queueMicrotask
    • Note that process.nextTick has a function signature of process.nextTick(fn, ...fnArgs) (passing arguments as rest parameters), while queueMicrotask does not have ...fnArgs.
  • Remove the nextTick implementation and test cases
    • The PR also removes the nextTick from the process ponyfill and its type definition

Also a question: should we also replace setImmediate with queueMicrotask? Both of them will flush the callback after microtask queue has emptied.

@SukkaW

This comment was marked as spam.

@SukkaW

This comment was marked as spam.

@G-Rath G-Rath changed the title refactor(#934): switch from process.nextTick() to queueMicrotask() refactor: switch from process.nextTick() to queueMicrotask() Sep 15, 2023
@SukkaW
Copy link
Contributor Author

SukkaW commented Oct 7, 2023

@streamich @G-Rath Kindly request your attention with a friendly ping. It has been more than a month since I opened this PR. During this time, I've rebased my PR on top of your pushes many times.

I would greatly appreciate it if you could spare some time to review it, at your convenience, and provide your valuable feedback.

I know it would be annoying for you to receive a "ping" like this. However, considering that almost every memfs issue is enforcing the "PR is welcome" policy, ignoring my "All checks have passed" PR doesn't look right.

@G-Rath
Copy link
Collaborator

G-Rath commented Oct 7, 2023

@SukkaW you've rebased this 2-3 times at most - I do my best to keep on top of the open source repositories I help maintain but my time is finite.

This pull request is not one I'd consider critical which is why I have not jumped on it (as it will probably be landed as a refactor it's not even going to be released by itself); your weekly spamming of me as well as sass in your latest commit did not help make me enthusiastic about your work here.

As it happens I will have time tomorrow I can spend landing this but do not take that as a promise since my personal life takes priority.

Also please don't take this as a request not to continue contributing - I appreciate the support, so long as you understand the priorities; likewise feel free to mention if a particular PR would resolve a particular issue you're having, to get that prioritized.

Copy link
Owner

@streamich streamich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@streamich streamich merged commit c955827 into streamich:master Oct 7, 2023
10 checks passed
@streamich
Copy link
Owner

🎉 This PR is included in version 4.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch to queueMicrotask()
3 participants