feat(Upgrade): add scope flag to the upgrade command #3190
Conversation
There was a problem hiding this comment.
Maybe an RFC would have been a good start, but it's simple enough and I can see the use case so I guess we can talk about this here (@bestander).
src/cli/commands/upgrade.js
Outdated
|
|
||
| const addArgs = args.map((dependency) => { | ||
| const remoteSource = allDependencies[dependency]; | ||
| console.log(allDependencies); |
There was a problem hiding this comment.
Sorry, my mistake...
src/cli/commands/upgrade.js
Outdated
| if (remoteSource && PackageRequest.getExoticResolver(remoteSource)) { | ||
| return remoteSource; | ||
| if (flags.scope) { | ||
| const searchPattern = new RegExp(`^${flags.scope}`); |
There was a problem hiding this comment.
1/ We probably want to run a check to make sure that flags.scope is a valid scope, and abort otherwise
2/ The regex above will make -S @angular upgrade @angular, but also @angular-community etc.
There was a problem hiding this comment.
I will implement a check.
src/cli/commands/upgrade.js
Outdated
| addArgs = args.map((dependency) => { | ||
| const remoteSource = allDependencies[dependency]; | ||
|
|
||
| console.log(remoteSource); |
src/cli/commands/upgrade.js
Outdated
| addArgs.push(remoteSource); | ||
| } | ||
|
|
||
| addArgs.push(dependency); |
There was a problem hiding this comment.
Shouldn't that be in an else statement?
__tests__/commands/upgrade.js
Outdated
| expect(pkg.dependencies['@angular/core']).not.toEqual('^2.4.9'); | ||
| expect(pkg.dependencies['left-pad']).toEqual('^1.0.0'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Please also add a test to make sure that -S @angular doesn't upgrade @angular-whatever
There was a problem hiding this comment.
I will fix the test issue and add this test case.
| @@ -27,16 +28,37 @@ export async function run( | |||
| peerDependencies, | |||
| } = await config.readRootManifest() || {}; | |||
| const allDependencies = Object.assign({}, peerDependencies, optionalDependencies, devDependencies, dependencies); | |||
| let addArgs = []; | |||
There was a problem hiding this comment.
Regarding the global workflow of the code, I'd prefer using ES5 array functions:
let addArgs = Object.keys(allDependencies);
if (flags.scope) {
addArgs = addArgs.filter(arg => {
return ...;
});
}
addArgs = addArgs.map(arg => {
return ...;
});There was a problem hiding this comment.
Yes, I will do that.
|
Please also note that your test isn't passing. |
|
I will fix the issues tomorrow. |
|
I have fixed te issues (@arcanis). |
src/cli/commands/upgrade.js
Outdated
| if (remoteSource && PackageRequest.getExoticResolver(remoteSource)) { | ||
| return remoteSource; | ||
| } | ||
| if (/^@\S*\/$/g.test(flags.scope)) { |
There was a problem hiding this comment.
Use [a-zA-Z0-9-][a-zA-Z0-9_.-]* instead of \S*, this way we ensure that the scope is a valid scope according to npm rules (and we won't risk any schenanigans with extra /).
There was a problem hiding this comment.
Okay, I will change it.
src/cli/commands/upgrade.js
Outdated
| return remoteSource; | ||
| } | ||
| if (/^@\S*\/$/g.test(flags.scope)) { | ||
| const searchPattern = new RegExp(`^${flags.scope}`); |
There was a problem hiding this comment.
It's better to check directly with String.prototype.startsWith (otherwise you'll have issues with the . character)
There was a problem hiding this comment.
Can you make an example, because I don't understand the issue?
|
The Travis test is failing (apparently, |
|
I fixed the issues (@arcanis). |
|
Thanks ! 👍 |
The
upgradecommand now supports the-S, --scope <scope>flag. If the flag is set and a scope is provided yarn upgrade only the packages, which belong to the specified scope.It is very usefull when you have packages under the
@angularscope, so you can upgrade all of them at the same time without touching the other packages.I have tested the command in multiple projects and I have written some test code.
I hope you understand the use case and integrate the pull request.