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 #1325 - setting errorLevel in workspaces doesn't work as expected #1326

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

regevbr
Copy link
Contributor

@regevbr regevbr commented Sep 5, 2023

fix #1325

@regevbr
Copy link
Contributor Author

regevbr commented Sep 5, 2023

@raineorshine I would appreciate your CR

Copy link
Owner

@raineorshine raineorshine left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

src/index.ts Outdated
}
}, Promise.resolve({} as Index<PackageFile> | PackageFile))
if (options.json) {
printJson(options, analysis)
}
if (totalUpgradedCount > 0 && options.errorLevel === 2) {
Copy link
Owner

@raineorshine raineorshine Sep 5, 2023

Choose a reason for hiding this comment

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

I think we can just check if the analysis is empty. Then we don't have to alter the return type or add other variables.

See the someUpgraded variable in the npmInstall function, as analysis is nested one extra level in workspace mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok perfect changed it - it is a much smaller change now. I wasn't too sure about the structure of the response

Copy link
Owner

@raineorshine raineorshine left a comment

Choose a reason for hiding this comment

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

Thanks! That looks better. Just one question for you.

src/index.ts Outdated
@@ -209,6 +209,7 @@ async function runUpgrades(options: Options, timeout?: NodeJS.Timeout): Promise<
...rcConfig,
packageFile: packageInfo.filepath,
workspacePackages,
errorLevel: 1,
Copy link
Owner

Choose a reason for hiding this comment

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

Why set errorLevel here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we do not, it will throw if there are upgrades, which is the current behaviour we are trying to fix

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, thanks. It seems wrong that we have to hardcode this here when the default value of errorLevel is already 1. Maybe we can consolidate the workspace and non-workspace logic to avoid this?

I will need to take a closer look at how it's handled to know. I'm not available as much right since I'm traveling, but I can check it out soon.

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 default is indeed 1, but in my use case, we pass 2, thus causing it to exist on the first workspace that needs updating. This will ensure that it will not error on first workspace.
If you want, we can instead pass a flag to runLocal that instructs to not exit on upgrades needed. What do you think?

Copy link
Owner

Choose a reason for hiding this comment

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

If we need to override the errorLevel passed to runLocal, then the exit logic is probably in the wrong place. Without having looked at the code yet, I'm thinking we need to return early from runLocal but only exit the process at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I see your point - I will make the change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done - cleaner code now

Copy link
Owner

@raineorshine raineorshine left a comment

Choose a reason for hiding this comment

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

💯

@raineorshine raineorshine merged commit 9d117b3 into raineorshine:main Sep 7, 2023
8 of 9 checks passed
@regevbr regevbr deleted the 1325 branch September 8, 2023 07:38
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.

setting errorLevel in workspaces doesn't work as expected
2 participants