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

[feature] [may break .yarnrc for older versions] Ability to pass command CLI arguments via configuration in .yarnrc #3033

Merged
merged 8 commits into from
Apr 6, 2017

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented Apr 3, 2017

Summary

This PR opens the way to share configuration between multiple projects via the use of the .yarnrc
files. The current design it pretty simple: it simply adds the supported options from the yarnrc file at the beginning of the command line.

Syntax

Adding the following into a yarnrc path in your cwd or any path above will automatically add the check files option when running yarn install (but not any other command).

$> cat .yarnrc
--install.check-files true

When the command name is missing, the argument will be added to every command you call. For example, the following will change the cache path everywhere:

$> cat .yarnrc
--cache-folder /tmp/yarn-cache/

$> yarn cache dir
/tmp/yarn-cache/v1

Since the arguments are added at the beginning of the command line, every argument you manually specify when calling the binary will override the yarnrc-providen ones. For example:

$> cat .yarnrc
--cache-folder /tmp/yarn-cache/

$> yarn cache dir --cache-folder=/tmp/foobar
/tmp/foobar/v1

Notes

  • The yarnrc files are extracted starting from your current working directory - not the package.json directory.

  • The yarnrc is currently parsed synchronously. I believe this is not an issue, since it's quite literally the very first thing we need to do when booting the application (even before the command line can be parsed), so there's really nothing we could run concurrently anyway.

  • The yarnrc is read in another file (https://github.com/yarnpkg/yarn/blob/master/src/registries/yarn-registry.js). I haven't changed how it works there, since I wanted my first draft to have as few changes as possible, but it's something that should probably be fixed so that the code only use one path to manage the yarnrc files.

Test plan

I still have to write a test to be sure that it works as expected, but the existing ones don't break.

@arcanis arcanis requested a review from bestander April 3, 2017 15:42
@arcanis arcanis self-assigned this Apr 3, 2017
src/rc.js Outdated
'mutex',

'no-emoji',
'har',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should keep the list of synced args and .yarnrc properties?
I wonder if we can make it a dynamic setting that would be generic

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been wondering the same thing, but I'm a bit worried about the potential effects of letting the users put any argument into their rc files :/ For example, --force in the yarnrc seems like a bad idea.

@bestander
Copy link
Member

bestander commented Apr 4, 2017 via email

@arcanis
Copy link
Member Author

arcanis commented Apr 4, 2017

It makes sense for most of the other parameters to be used as-is (for example, I'd prefer to write cache-folder rather than argument-cache-folder). But maybe we could only prefix a few parameters (on the top of my head, --force, --verbose, --json) and let the other ones be used without requiring such a prefix.

@bestander
Copy link
Member

bestander commented Apr 4, 2017 via email

@arcanis
Copy link
Member Author

arcanis commented Apr 4, 2017

Shouldn't that be avoided? Even if arguments are stored in a separate "namespace", it sounds confusing to have two similarly named keys that unlock two different behaviors.

My initial approach was to consider that semantically speaking, nothing prevented the values stored in an rc file from being specified as a command line argument (in fact, the default behavior of the rc module is to return the result of applying the command line arguments on the content of the rc files - a behavior I've disabled so that we can have more control on how to do it). Do you think the yarnrc parameters should be completely dissociated from the command line arguments?

@bestander
Copy link
Member

bestander commented Apr 4, 2017 via email

@bestander
Copy link
Member

bestander commented Apr 4, 2017

A few use cases to consider:

yarn check --integrity --check-files 
yarn install --mutex network:8084 --check-files
yarn install --cache-folder /var/tmp/caches --production --skip-integrity-check

@arcanis
Copy link
Member Author

arcanis commented Apr 4, 2017

I was thinking about a syntax:

--production
--cache-folder /var/tmp/caches
--install.skip-integrity-check

Basically, all command line flags would be prefixed by -- (so we wouldn't have to whitelist them, and their effects would be a bit clearer), and they could be restricted to a command by prefixing them with the command name (so --install.foobar would only add the --foobar option when calling the install command). It would make it easier to avoid adding arguments that don't make sense, and would prevent mistakes with flags that may have different meanings depending on the commands being run (--force).

The main issue is that I don't find it very pretty ... Would probably be better if the yarnrc file was written with YAML or similar.

@bestander
Copy link
Member

bestander commented Apr 4, 2017 via email

@arcanis
Copy link
Member Author

arcanis commented Apr 5, 2017

Should be mostly fine, I'm waiting for the tests. The only issue I had is that the rc module I've used doesn't pass the yarnrc location to the custom parser function, which makes it impossible to resolve the paths stored inside the yarnrcs relatively to their locations. I'll try to see if they would merge a PR (related issue: dominictarr/rc#61), but in the meantime I just prevent relative path from being read at all from the yarnrc files (absolute paths are fine), which should be enough for a first iteration. WDYT?

src/rc.js Outdated
@@ -0,0 +1,87 @@
/* @flow */

import rc from 'rc';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we use require() for npm dependencies

@bestander
Copy link
Member

I think you don't need to bother about paths relative to .yarnrc.
Because .yarnrc files can be cascading it may be hard to keep track of the paths relative to all the files.
Instead the path should be relative to CWD IMHO.

@bestander bestander changed the title Loads extra arguments from the yarnrc files [feature] Ability to pass command CLI arguments via configuration in .yarnrc Apr 6, 2017
@bestander bestander merged commit a5a5915 into yarnpkg:master Apr 6, 2017
@arcanis arcanis mentioned this pull request Apr 6, 2017
@bestander bestander changed the title [feature] Ability to pass command CLI arguments via configuration in .yarnrc [feature] [may break .yarnrc for older versions] Ability to pass command CLI arguments via configuration in .yarnrc Apr 6, 2017
@felixfbecker
Copy link

The yarnrc files are extracted starting from your current working directory - not the package.json directory.

This may be true if only yarn is run in the process, but not if yarn is used programmatically as part of an application. Blocking the whole app seems very dangerous in that scenario. I would appreciate it if you could keep this use case in mind for future improvements.

@arcanis
Copy link
Member Author

arcanis commented Apr 7, 2017

I'm not sure I see what you mean - do you have an example in mind?

@felixfbecker
Copy link

You can install and require() locally and use it e.g. as part of a web server that does code intelligence. Of course you could use a child process, but that has the overhead of spinning up a V8 instance.

@bestander
Copy link
Member

I think we need to improve Yarn require-ability for programmatic use.
Anyone volunteers to start an RFC?

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.

3 participants