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

Unwind recursion in loopAsync when possible #2923

Merged
merged 3 commits into from
Jan 18, 2016
Merged

Unwind recursion in loopAsync when possible #2923

merged 3 commits into from
Jan 18, 2016

Conversation

taion
Copy link
Contributor

@taion taion commented Jan 18, 2016

For #2922

@taion taion mentioned this pull request Jan 18, 2016
@taion
Copy link
Contributor Author

taion commented Jan 18, 2016

cc @mhart

@mhart
Copy link

mhart commented Jan 18, 2016

This is a huge win for me – cut the stacks in our profiling down by at least 25% – and we only have 20 routes, so those with more routes will see even more benefits.

Thanks!

@taion taion changed the title [WIP] Unwind recursion in loopAsync when possible Unwind recursion in loopAsync when possible Jan 18, 2016
@taion
Copy link
Contributor Author

taion commented Jan 18, 2016

Great, thanks. I'll let this sit a little more in case there are any more comments – we can't cut the next React Router RC without a new history RC anyway.

@taion taion added this to the v2.0.0-final milestone Jan 18, 2016
@timdorr
Copy link
Member

timdorr commented Jan 18, 2016

Not sure why coveralls isn't coming back OK, but this looks good to me.

timdorr added a commit that referenced this pull request Jan 18, 2016
Unwind recursion in loopAsync when possible
@timdorr timdorr merged commit d8eed68 into remix-run:master Jan 18, 2016
@taion taion deleted the loopAsync-unwind-recursion branch January 18, 2016 20:39
@gcorne
Copy link
Contributor

gcorne commented Feb 16, 2016

@timdorr would you be up for back porting this to 1.0.x? I bumped into an InternalError: too much recursion on FF for Windows and this change fixes the issue for me.

@timdorr
Copy link
Member

timdorr commented Feb 16, 2016

I'm not sure of our policy on patches for 1.0, but a PR would certainly make it more likely to happen 😄

@taion
Copy link
Contributor Author

taion commented Feb 16, 2016

I don't think we should – 2.x is essentially backward compatible.

@timdorr
Copy link
Member

timdorr commented Feb 16, 2016

@gcorne I'm not sure of your particular use case, but @taion is right and upgrading should be painless. Just bump the version and start hunting for deprecation warnings. You can automate that with codemods too: https://github.com/reactjs/rackt-codemod

@gcorne
Copy link
Contributor

gcorne commented Feb 17, 2016

@gcorne I'm not sure of your particular use case, but @taion is right and upgrading should be painless.

Okay, I'll try that. Upgrading to 1.0.x was kind of painful, so I didn't expect the upgrade to 2.0.x to be painless. I'll give it a spin and see how it goes.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants