Fix: Prevents interpreting run options as yarn options #4152

arcanis · 2017-08-11T14:16:35Z

Summary

Running the following command doesn't have the expected effect, because the --no-coverage option is interpreter by Yarn instead of being forwarded to the script. This patch fix this.

$> yarn test-only --no-coverage     # currently bad
$> yarn test-only -- --no-coverage  # currently good

This is a breaking change: it will change some edge-case behaviors.

BYK · 2017-08-11T15:15:40Z

I remember talking about this but couldn't find the issue. Do you know if we have an open or closed issue around this? The problem with this approach is it makes yarn take no arguments at all such as --prefix or --modules-folder etc. so although I like the easier and cleaner invocation, I have serious doubts about whether we should do this or not.

BYK · 2017-08-11T15:15:50Z

Also, tests or didn't happen.

arcanis · 2017-08-11T16:03:02Z

Do you know if we have an open or closed issue around this?

No, this is an issue reported by the Oculus people, and something I personally struggle with on a daily basis. :p

The problem with this approach is it makes yarn take no arguments at all such as --prefix or --modules-folder.

Not really an issue since the only potential option concerned is --prefix, which is not implemented and has its own set of issues + the solution is obvious: any option before the script name would have to be yarn options. We currently don't need this behavior, tho.

kaylie-alexa · 2017-08-13T00:55:31Z

Although it's convenient to not have to use the --, this would change the behavior from npm and how we write scripts inside package.json, so I'd prefer it if we could keep them the same.

arcanis · 2017-08-14T10:02:28Z

Hm I don't know, this behavior really is a pain, especially since it is completely silent, which means that unless people actually look at the command executed, they won't see that the options have been ignored. It's potentially dangerous (just imagine something like yarn rm / --dry-run being executed).

A less invasive possibility would be to only add this behavior when running yarn <pkg>. Running yarn run <pkg> would still require the --. I don't really like this because it causes an unnecessary split between the two commands behaviors, but maybe it's still better than the alternative.

cpojer · 2017-08-15T07:53:26Z

I think I'm on board with this change, but agree that we need tests for it. We had an internal discussion about this at FB previously and would like to make sure other people are involved in a vote for this feature. I seem to remember that there were some issues with this approach that made this less workable, but I do think that yarn --options run --foo makes a lot of sense to overall and will definitely be a better UX for beginners. Can we make this work at all in a backwards compatible way at all?

@kaylieEB I think diverging here is ok, given that in scripts you'd write "yarn run …" instead of "npm run" or a generic command, like just "run", so it's clear that you are using either tool.

arcanis · 2017-08-15T10:43:20Z

I don't think we can - the only way I could see this working would be to add some kind of "if the command line doesn't begins with --, add one" (so that we wouldn't break people already using --), but that seems extremely confusing and unexpected.

BYK · 2017-08-15T10:45:02Z

I think the point we ended up is pretty great so 👍 . Thanks everyone, especially @arcanis. We need to be very loud and clear about this change though.

cpojer · 2017-08-15T10:48:54Z

I think that's confusing and will break projects with people on different versions of Yarn. Can we add a deprecation warning to 1.0 if the command starts with <bin> run -- and continue on with the old behavior? I understand it is confusing, but I think breaking this in the 1.0 release will cause more pain than it provides value.

kaylie-alexa · 2017-08-15T18:26:59Z

Can we add a deprecation warning to 1.0 if the command starts with run -- and continue on with the old behavior?

I think if this could be supported, it would be great since devs don't have to alter their current use habits, similar to how we support both yarn [command] and yarn run [command]. If not, definitely a big warning would be helpful.

arcanis · 2017-08-18T14:31:28Z

@BYK @kaylieEB good for review - added a deprecation warning when manually using --

cpojer · 2017-08-18T16:13:46Z

src/reporters/lang/en.js

@@ -188,6 +188,8 @@ const messages = {

  execMissingCommand: 'Missing command name.',

+  dashDashDeprecation:
+    "Using -- to pass arguments to your scripts isn't required anymore. Doing this may cause issues in future versions.",


How about: "Yarn does not require `--` to pass arguments to scripts. In a future version of Yarn, passing `--` will fail with an error." ?

It won't fail, it will just be forwarded to the binary as-is.

$> yarn test -- --no-coverage

will become

$> jest -- --no-coverage

instead of (as before this PR, and until the deprecation wears off)

$> jest --no-coverage

This is important because in some case you really want to prevent something from being interpreted as an option.

Right, in that case then, maybe this: "From Yarn 1.0, scripts do not require `--` to be passed. In a future version of Yarn, passing `--` will be forwarded to the script directly." ?

Much better than the current text, thanks 👍

kaylie-alexa · 2017-08-18T16:34:14Z

src/cli/index.js

@@ -403,7 +418,7 @@ export default function start() {
  const doubleDashIndex = process.argv.findIndex(element => element === '--');
  const startArgs = process.argv.slice(0, 2);
  const args = process.argv.slice(2, doubleDashIndex === -1 ? process.argv.length : doubleDashIndex);
-  const endArgs = doubleDashIndex === -1 ? [] : process.argv.slice(doubleDashIndex + 1, process.argv.length);
+  const endArgs = doubleDashIndex === -1 ? [] : process.argv.slice(doubleDashIndex, process.argv.length);


can you do process.argv.slice(doubleDashIndex) since it'll just default to length of the array?

Sure thing 👍

kaylie-alexa · 2017-08-18T16:39:15Z

src/cli/index.js

+  // we using "yarn <script> -abc" or "yarn run <script> -abc", we want -abc to be script options, not yarn options
+  if (command === commands.run) {
+    if (endArgs.length === 0) {
+      endArgs = ['--', ...args.splice(1, args.length)];


same for here? args.splice(1)?

kaylie-alexa

looks great!

BYK

This looks pretty nice but it is also a hack on the existing system that relies on -- style still. Do we have a long-term plan for making this more maintainable?

BYK · 2017-08-21T14:58:23Z

__tests__/integration.js

@@ -73,6 +73,62 @@ test('--mutex network', async () => {
  ]);
 });

+// Windows doesn't have the "echo" utility


How about node -p -e "process.argv.slice(1)" for cross-platform compatibility? ;)

BYK · 2017-08-21T15:01:18Z

__tests__/integration.js

+    const stdoutPromise = misc.consumeStream(stdout);
+    const stderrPromise = misc.consumeStream(stderr);
+
+    const [stdoutOutput, stderrOutput] = await Promise.all([stdoutPromise, stderrPromise]);


Is there an advantage to this indirection instead of just doing

const stdoutOutput = await misc.consumeStream(stdout); const stderrOutput = await misc.consumeStream(stderr);

Problem with this writing is that it doesn't work, because the stdin and stdout streams are started and stopped as soon as the event loop gets a chance, ie. before the second line gets executed. Before of this, the second consumeStream call never receives the end event, and never returns. The synchronous version makes sure we attach both of our listeners before allowing the event loop to continue.

Got it. I think this deserves a comment. Even better, find a way that doesn't require this workaround (not necessarily in this patch). Maybe use something else rather than the stream consumer? This may also help: https://nodejs.org/api/stream.html#stream_readable_resume

BYK · 2017-08-21T15:02:09Z

__tests__/integration.js

+
+    const [stdoutOutput, stderrOutput] = await Promise.all([stdoutPromise, stderrPromise]);
+
+    expect(stdoutOutput.toString().trim()).toEqual('--opt');


Are we testing the "$ " echo string here? If so we may wanna add a comment since this may change.

No, because we run with YARN_SILENT: 1, so Yarn doesn't print the yarn run header.

Oh, nice. Then what are we testing here? I'm confused :D

BYK · 2017-08-21T15:03:27Z

__tests__/integration.js

+    const stdoutPromise = misc.consumeStream(stdout);
+    const stderrPromise = misc.consumeStream(stderr);
+
+    const [stdoutOutput, stderrOutput] = await Promise.all([stdoutPromise, stderrPromise]);


Lots of repeated code here. You may clean it up with beforeEach and afterEach hooks with a describe('run with arguments', block.

BYK · 2017-08-21T15:04:34Z

src/cli/index.js

@@ -110,6 +110,16 @@ export function main({
    command = commands.run;
  }

+  let warnAboutRunDashDash = false;
+  // we using "yarn <script> -abc" or "yarn run <script> -abc", we want -abc to be script options, not yarn options


we are using

BYK · 2017-08-21T15:08:16Z

src/cli/index.js

@@ -171,6 +181,11 @@ export function main({
  //
  const run = (): Promise<void> => {
    invariant(command, 'missing command');
+
+    if (warnAboutRunDashDash) {
+      reporter.warn(reporter.lang('dashDashDeprecation'));


Arguably, this should be an info message, not a warning since with the --strict option it will break and I don't think should break just because you are still using the harmless -- thing. What do you think?

Makes sense, but the PR that will implement the --strict feature will need to revisit all the loglevels anyway.

Eh, fair point. I'd still get certain things "right" from the start. That said we don't show info messages in normal mode, are we? Then I'm okay with warnings.

arcanis · 2017-08-21T15:21:12Z

@BYK Long term plan is to switch to a different command line parser more suited to our use cases. I have it on my backlog, but time has been a scarce resource lately :p

BYK · 2017-08-21T18:49:45Z

I have it on my backlog, but time has been a scarce resource lately :p

Shall we make it explicit by creating an issue?

arcanis · 2017-08-22T10:47:21Z

@BYK #4225

**Summary** Fixes #4383. This patch makes argument parsing a bit tidier, and starts supporting `yarn --silent custom-script` style commands as initiall intended by #4152. **Test plan** Existing unit tests.

**Summary** Fixes #4383. This patch makes argument parsing a bit tidier, and starts supporting `yarn --silent custom-script` style commands as initiall intended by #4152. **Test plan** Existing unit tests. Should ideally add a few more.

* Prevents interpreting run options as yarn options * Implements a deprecation warning * Adds tests * Feedback * Wording * Enables a test on Linux * Disables the tests again on Windows

**Summary** Fixes yarnpkg#4383. This patch makes argument parsing a bit tidier, and starts supporting `yarn --silent custom-script` style commands as initiall intended by yarnpkg#4152. **Test plan** Existing unit tests. Should ideally add a few more.

arcanis mentioned this pull request Aug 15, 2017

Add cwd flag #4174

Merged

arcanis force-pushed the yarn-run branch 2 times, most recently from a1549ce to e620c48 Compare August 17, 2017 17:06

BYK self-assigned this Aug 18, 2017

arcanis force-pushed the yarn-run branch 2 times, most recently from e620c48 to 8dff818 Compare August 18, 2017 12:49

Maël Nison added 3 commits August 18, 2017 13:58

Prevents interpreting run options as yarn options

8b08820

Implements a deprecation warning

680fb69

Adds tests

b6ef55a

arcanis force-pushed the yarn-run branch from 8dff818 to b6ef55a Compare August 18, 2017 12:58

cpojer reviewed Aug 18, 2017

View reviewed changes

kaylie-alexa reviewed Aug 18, 2017

View reviewed changes

Feedback

91a555b

kaylie-alexa approved these changes Aug 18, 2017

View reviewed changes

arcanis force-pushed the yarn-run branch from e3e7b3e to 7482fed Compare August 18, 2017 17:14

Wording

8ae556d

arcanis force-pushed the yarn-run branch from 7482fed to 8ae556d Compare August 21, 2017 09:55

BYK approved these changes Aug 21, 2017

View reviewed changes

Enables a test on Linux

5ae2aab

arcanis force-pushed the yarn-run branch from b2cf643 to 5ae2aab Compare August 22, 2017 10:29

Disables the tests again on Windows

8d83c4e

arcanis merged commit 52a35c9 into yarnpkg:master Aug 22, 2017

This was referenced Sep 10, 2017

Bug: silent mode through --silent not working with yarn run #4383

Closed

Add --silent flag to CLI run command #788

Closed

BYK mentioned this pull request Sep 10, 2017

Fix: fix incorrect argument parsing #4384

Merged

BYK mentioned this pull request Sep 12, 2017

yarn run passes command line arguments to the script that is executed #4375

Closed

nalla mentioned this pull request Sep 28, 2017

yarn-run: Remove obsolete argument separator cake-contrib/Cake.Yarn#13

Merged

rhendric mentioned this pull request Sep 29, 2017

fix(run): improve escaping for script arguments #4135

Merged

kaylie-alexa mentioned this pull request Feb 26, 2018

yarn run command --production doesn't set NODE_ENV like npm does #5395

Closed

cgravill mentioned this pull request Mar 9, 2018

CLI commands in Fable 2 fable-compiler/Fable#1361

Closed

kitfit-dave mentioned this pull request Nov 23, 2018

Unable to pass "end of options" marker (--) to binary in "yarn run" #6717

Closed

oxyc mentioned this pull request Jun 18, 2019

Fix npm run start script options not forwarded with npm roots/sage#2205

Merged

SpecLad mentioned this pull request Oct 26, 2022

Remove -- from yarn run commands cvat-ai/cvat#5173

Merged

3 tasks


		const [stdoutOutput, stderrOutput] = await Promise.all([stdoutPromise, stderrPromise]);

		expect(stdoutOutput.toString().trim()).toEqual('--opt');

Fix: Prevents interpreting run options as yarn options #4152

Fix: Prevents interpreting run options as yarn options #4152

Uh oh!

Conversation

arcanis commented Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BYK commented Aug 11, 2017

Uh oh!

BYK commented Aug 11, 2017

Uh oh!

arcanis commented Aug 11, 2017

Uh oh!

kaylie-alexa commented Aug 13, 2017

Uh oh!

arcanis commented Aug 14, 2017

Uh oh!

cpojer commented Aug 15, 2017

Uh oh!

arcanis commented Aug 15, 2017

Uh oh!

BYK commented Aug 15, 2017

Uh oh!

cpojer commented Aug 15, 2017

Uh oh!

kaylie-alexa commented Aug 15, 2017

Uh oh!

arcanis commented Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arcanis Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaylie-alexa left a comment

Choose a reason for hiding this comment

Uh oh!

BYK left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BYK Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BYK Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

arcanis commented Aug 11, 2017 •

edited

Loading

arcanis commented Aug 18, 2017 •

edited

Loading

arcanis Aug 18, 2017 •

edited

Loading

BYK Aug 21, 2017 •

edited

Loading

BYK Aug 21, 2017 •

edited

Loading