fix #1325 - setting errorLevel in workspaces doesn't work as expected #1326
Conversation
|
@raineorshine I would appreciate your CR |
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.
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.
Ok perfect changed it - it is a much smaller change now. I wasn't too sure about the structure of the response
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.
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.
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.
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.
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.
I think I see your point - I will make the change
There was a problem hiding this comment.
Done - cleaner code now
fix #1325