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

Use setImmediate consistently over nextTick #904

Merged
merged 1 commit into from
Sep 25, 2015
Merged

Use setImmediate consistently over nextTick #904

merged 1 commit into from
Sep 25, 2015

Conversation

megawac
Copy link
Collaborator

@megawac megawac commented Sep 7, 2015

Reminded of this by #903

This probably justifies a patch release... previously on gitter

joystick Now I'm facing some (node) warning: Recursive process.nextTick detected. This will break in the next version of node. Please use setImmediate for recursive deferral.
megawac hey @joystick I think you may of found a bug/inconsistency. Want to submit a pull request to the repo changing async.nextTick to async.setImmediate?
joystick @megawac I'm not yet ready for PR submission, since I don't know how to fix this. But I will collect info to reproduce the case. This issue is about iterating over array of 1k to 20k elements. Iterator make call to SAP server to find account data information. When I limit array to 200-500 elements no warning issued. if 1k+ I receive warnings about nextTick.
megawac @joystick replace nexttick with async.
joystick @megawac I don't call nexttick in my code. please advise where to replace it.
I'm looking for advise if my application has correct logic in general.
megawac I'm referring to just replacing all uses of nextTick in async.js (and sending a PR)

/cc @joystick

@aearly
Copy link
Collaborator

aearly commented Sep 8, 2015

👍 setImmediate is better that nextTick in these cases.

megawac added a commit that referenced this pull request Sep 25, 2015
Use setImmediate consistently over nextTick
@megawac megawac merged commit 22e89cb into master Sep 25, 2015
@megawac megawac deleted the setImm branch September 25, 2015 09:10
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.

None yet

2 participants