fix(@angular/cli): pass arguments to all targets #12673
Conversation
|
ping @clydin |
There was a problem hiding this comment.
Thank you for the contribution.
This is unfortunately working around the problem and not actually solving the root cause. The algorithm should not be mutating the input array. Can you remove the mutations instead?
Reassigning arguments should also be avoided whenever possible.
|
@clydin this is a fair comment - I'll have a look at this tonight |
|
@clydin and now I remember why I didn't do this in the first place, not quite the simple fix I had hoped! The file is well covered by unit tests though, so I'm pretty confident that the behaviour has not been changed (aside from stopping the mutation) |
|
Hi @clydin any chance you could review this soon? Cheers |
| @@ -270,30 +284,73 @@ export function parseArguments(args: string[], options: Option[] | null): Argume | |||
| const ignored: string[] = []; | |||
| const errors: string[] = []; | |||
|
|
|||
| for (let arg = args.shift(); arg !== undefined; arg = args.shift()) { | |||
| for (let i = 0; i < args.length; i++) { | |||
There was a problem hiding this comment.
Instead of using 'i' which is already used below, use something similar to argIndex. Eliminates the need to change the other loop to use j.
| leftovers, | ||
| ignored, | ||
| errors, | ||
| }: { | ||
| arg: string, |
There was a problem hiding this comment.
change the first argument from being the actual argument to the index of the argument. You can then access the current argument and next argument by using the existing args array.
Also, have it return the new index. This would eliminate the extra conditionals present in the calling function.
There was a problem hiding this comment.
I don't think this is possible, as the value for "arg" is computed rather than from the array for one of the function calls (I'll leave a comment pointing out the call in question)
| const f = '--' + flag + arg.slice(j + 1); | ||
|
|
||
| const consumedNextArg = _assignOption({ | ||
| arg: f, |
There was a problem hiding this comment.
For this usage of _assignOption we pass in a computed value rather than a value from the args array
|
Hi @mnahkies! This PR has merge conflicts due to recent upstream merges. |
When running a command with args against multiple targets, all targets should be given the args. As parseArguments was mutating the passed args array this wasn't the case. Fix by not mutating the array. This was especially noticeable when using the `ng lint --fix` command on a newly generated project, as files in the app target would be fixed, but e2e target would be only be linted (with no fix) Possibly closes angular#10657, angular#10656, angular#11005
mnahkies
force-pushed
the
fix-options-for-multi-target-commands
branch
from
7e86769 to
54bd5ab
Compare
|
Rebased on current master |
|
This would also fix #13111 |
clydin
added
target: patch
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
When running a command with args against multiple targets, all targets should be given the args. As parseArguments was mutating the passed args array this wasn't the case.
Fix by making a shallow clone of the array. This was especially noticeable when using the
ng lint --fixcommand on a newly generated project, as files in the app target would be fixed, but e2e target would be only be linted (with no fix).Possibly closes #10657, #10656, #11005