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

Avoid blowing stack when checking if a failed dependency is required or optional #1457

Closed
wants to merge 1 commit into from

Conversation

everett1992
Copy link

Avoid blowing stack when checking if a failed dependency is required or optional

I see this issue in our private repository, I can't provide a public reproduction.
If I install a package with a cycle in it's dependencies and one of the packages in the cycle fails npm will error with Maximum call stack size exceeded and a track trace pointing to failedDependency.

@everett1992 everett1992 requested a review from a team as a code owner June 24, 2020 02:47
failedDependency is depth first search for a path to a top-level or user-required package.
If failedDependency revisits a package it will recurse till it blows the call stack.
@ruyadorno ruyadorno added pr: needs tests requires tests before merging Release 6.x work is associated with a specific npm 6 release semver:patch semver patch level for changes labels Aug 14, 2020
@ruyadorno
Copy link
Contributor

hi @everett1992 thanks for your contribution! At this point we would def require a test demonstrating the failure in order to safely merge this change. 😊 I added the appropriate labels in case you or some other contributor have the time to put together some tests!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: needs tests requires tests before merging Release 6.x work is associated with a specific npm 6 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants