-
Notifications
You must be signed in to change notification settings - Fork 331
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
Conversation
@raineorshine I would appreciate your CR |
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why set errorLevel
here?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - cleaner code now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
fix #1325