Skip to content
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: Better npm_config_argv emulation #4479

Merged
merged 6 commits into from
Sep 18, 2017
Merged

Fix: Better npm_config_argv emulation #4479

merged 6 commits into from
Sep 18, 2017

Conversation

BYK
Copy link
Member

@BYK BYK commented Sep 15, 2017

Summary

Fixes #2226. Better emulates npm_config_argv by passing
process.argv.slice(2) as the original portion and both the
command name and the script name in cooked portion.

Test case

Added integration tests.

**Summary**

Fixes #2226. Better emulates `npm_config_argv` by passing
`process.argv.slice(2)` as the `original` portion and both the
command name and the script name in `cooked` portion.

**Test case**

Added integration tests.
@BYK
Copy link
Member Author

BYK commented Sep 15, 2017

Added integration tests.

Will actually add these soon.

@ghost
Copy link

ghost commented Sep 15, 2017

Thanks @BYK, this helps #2226 a little as you commented over there.

However, if I understand correctly (I've only recently started using yarn),
yarn <scriptname> is shorthand for yarn run <scriptname> so passing just process.argv.slice(2) as per summary above is not quite sufficient as npm_config_argv should ideally expand into the npm canonical form including the word "run" prior to <scriptname> in the (probably) cooked array.

You could go one better than npm and make that run-script as run is shorthand for run-script in npm so run-script is arguably the correct canonical form.

@BYK
Copy link
Member Author

BYK commented Sep 15, 2017

@indiescripter thanks for the feedback! So here's what happens:

  • original is always the untouched process.argv.slice(2)
  • cooked is the "interpreted" version, which would always be in the form ['run', 'script_name'] even when the command invoked is yarn script_name

The aim here is to make this variable more useful but not achieve 100% npm compatibility since that's not possible and is overly limiting.

TL;DR
Is this version making things easier and "better" for you or is it as useless as the current version?

@ghost
Copy link

ghost commented Sep 15, 2017

Thank you so much @BYK for taking such an interest in my situation.

So long as one way or another we end up with at least

['run', 'script_name'] in cooked

that is useful to/better for me.

This means my use case will work if the user types yarn run <scriptname>

It would be nice, though, as I suggested, if the user typed just the shorthand yarn <scriptname>
then yarn run <scriptname> or yarn run-script <scriptname> was automagically inferred but asking for that extra step might be stretching a friendship!

@BYK
Copy link
Member Author

BYK commented Sep 15, 2017

It would be nice, though, as I suggested, if the user typed just the shorthand yarn then yarn run or yarn run-script was automagically inferred but asking for that extra step might be stretching a friendship!

Oh, I already fixed some of this part :) When you run yarn foobar you still get ['run', 'foobar'] in cooked. That was the core of your issue, right?

@ghost
Copy link

ghost commented Sep 15, 2017

Fantastic, correct, thank you @BYK, that was the core issue!

I have a new project in development which I humbly feel quite excited about as it a somewhat novel approach to monorepo setups (i.e. multiple subpackages under a master, top-level, development package).

As you have taken a special interest in "Better npm_config_argv emulation", I would like to show my appreciation for your effort by giving you a heads-up well ahead of making any social media announcements about it.

The project, called buildverse is on npmjs.com at

https://www.npmjs.com/package/buildverse

The "novelty" of my approach is in the executable program npm-apply. This utility provides much flexibility in running package scripts in a depth-first traversal of a tree of subprojects. Judicial use of npm-apply allows the package scripts developer to implement preorder, inorder & postorder traversal patterns as described in the preceding Wikipedia link.

If you check out the GitHub repo for buildverse you will find that the npm-apply code makes use of 'npm_config_argv' to recursively run a npm ... command in subproject directories.

With your fix, I hope to be able to say "Works with yarn as well as npm" in the package README!

@BYK
Copy link
Member Author

BYK commented Sep 16, 2017

@indiescripter that's great, congrats! And I'm happy to be able to help :)

Looks like you may also contribute to yarn in the future since you seem to be interested in these kinds of things ;)

@buildsize
Copy link

buildsize bot commented Sep 16, 2017

This change will decrease the build size from 9.69 MB to 9.69 MB, a decrease of 250 bytes (0%)

File name Previous Size New Size Change
yarn-[version].noarch.rpm 835.17 KB 835.09 KB -74 bytes (0%)
yarn-[version].js 3.69 MB 3.69 MB -61 bytes (0%)
yarn-legacy-[version].js 3.74 MB 3.74 MB -48 bytes (0%)
yarn-v[version].tar.gz 839.19 KB 839.16 KB -31 bytes (0%)
yarn_[version]all.deb 635.16 KB 635.12 KB -36 bytes (0%)

@BYK BYK merged commit d64512c into master Sep 18, 2017
@BYK BYK deleted the npm_config_argv branch September 18, 2017 21:07
joaolucasl pushed a commit to joaolucasl/yarn that referenced this pull request Oct 27, 2017
**Summary**

Fixes yarnpkg#2226. Better emulates `npm_config_argv` by passing
`process.argv.slice(2)` as the `original` portion and both the
command name and the script name in `cooked` portion.

**Test case**

Added integration tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants