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

Bugfix: avoid infinite recursion in Bundle.sort() #800

Merged
merged 1 commit into from
Aug 1, 2016
Merged

Bugfix: avoid infinite recursion in Bundle.sort() #800

merged 1 commit into from
Aug 1, 2016

Conversation

phter
Copy link
Contributor

@phter phter commented Jul 28, 2016

No description provided.

@eventualbuddha
Copy link
Contributor

Can you add a test case that would exhibit the infinite recursion you mention? We'd like to be able to verify that this fixes a real problem.

@phter
Copy link
Contributor Author

phter commented Jul 28, 2016

I ran into this bug while working on a rather complex library, so no simple test case, sorry.
But please just look at the current code of findParent(), it seems simple enough:
The function never returns a value, yet it contains the line
if ( findParent( dependency ) ) return;
And it never checks if a module has already been visited, which doesn't go well with cyclical structures.

@phter
Copy link
Contributor Author

phter commented Jul 28, 2016

npm install
npm run build
--> Maximum call stack size exceeded at findParent ...
Do I get a t-shirt now?

rollup-test.zip

@TrySound
Copy link
Member

TrySound commented Aug 1, 2016

@phter Nice. Can you add it in PR?

@phter
Copy link
Contributor Author

phter commented Aug 1, 2016

@TrySound Like this?

@phter
Copy link
Contributor Author

phter commented Aug 1, 2016

@TrySound Uh, I messed up the branches of my forked repo, now some bogus commits appear in the log. That's annoying. Does it matter, though?

@TrySound
Copy link
Member

TrySound commented Aug 1, 2016

@phter Yep. Can you rebase and squash them (you don't need to create new PR for this)?

@phter
Copy link
Contributor Author

phter commented Aug 1, 2016

@TrySound ok, my git skills are somewhat lacking... but it seems to have worked.

@TrySound
Copy link
Member

TrySound commented Aug 1, 2016

Nice. Thanks. Will check it out soon.

@TrySound TrySound merged commit 0bb9a4c into rollup:master Aug 1, 2016
@TrySound
Copy link
Member

TrySound commented Aug 1, 2016

6ee27c6

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

3 participants