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

fix(core): catch all lock file parsing/pruning errors and provide fallback #15158

Merged
merged 4 commits into from Mar 7, 2023

Conversation

meeroslav
Copy link
Contributor

@meeroslav meeroslav commented Feb 21, 2023

Current Behavior

When npm stringification after pruning fails because some dependnecies could not be nested it throws an error, breaking the build.

Expected Behavior

Lock file stringification error should not break the build. We should rather return existing (potentially functional) lock file and notify user of potential issue and how to overcome it.

Related Issue(s)

Fixes #

@meeroslav meeroslav self-assigned this Feb 21, 2023
@vercel
Copy link

vercel bot commented Feb 21, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
nx-dev ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 7, 2023 at 3:28PM (UTC)

packages/nx/src/lock-file/npm-parser.ts Outdated Show resolved Hide resolved
@@ -527,7 +528,13 @@ function nestMappedPackages(
});

if (initialSize === nestedNodes.size) {
throw Error('Loop detected while pruning. Please report this issue.');
output.error({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this reported so low? Shouldn't we have a try catch around parseLockFile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This error happens on pruning and although it should not fail anymore, we still want to show error log if it does so we can track it.

This is the only remaining place we anticipate that error might occur.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should catch even unanticipated errors.

Throwing an Error here is fine. But we should catch it higher up if the whole operation is not necessary (/ there is a workaround)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be an error again right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intention was to try not break the lock file creation, as even without a few packages missing the installation might still work.

But I guess it's safer to just return the root lock file.

@@ -527,7 +528,13 @@ function nestMappedPackages(
});

if (initialSize === nestedNodes.size) {
throw Error('Loop detected while pruning. Please report this issue.');
output.error({
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should catch even unanticipated errors.

Throwing an Error here is fine. But we should catch it higher up if the whole operation is not necessary (/ there is a workaround)

@meeroslav meeroslav changed the title feat(core): replace pruning error with notification fix(core): catch all lock file parsing/pruning errors and provide fallback Feb 28, 2023
@@ -527,7 +528,13 @@ function nestMappedPackages(
});

if (initialSize === nestedNodes.size) {
throw Error('Loop detected while pruning. Please report this issue.');
output.error({
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be an error again right?

@FrozenPandaz FrozenPandaz merged commit 5d69c4e into nrwl:master Mar 7, 2023
12 of 14 checks passed
FrozenPandaz pushed a commit that referenced this pull request Mar 8, 2023
…lback (#15158)

Co-authored-by: Jason Jean <jasonjean1993@gmail.com>
(cherry picked from commit 5d69c4e)
nartc pushed a commit to nartc/nx that referenced this pull request Mar 9, 2023
…lback (nrwl#15158)

Co-authored-by: Jason Jean <jasonjean1993@gmail.com>
@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2023
@meeroslav meeroslav deleted the fix-error-message branch April 27, 2023 10:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
scope: core core nx functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants