Read config from ~/.yarnrc #5812

otaku · 2018-05-12T20:50:31Z

Given the following:

yarn config set @scope:registry https://registry.npmjs.org/
yarn config set //registry.npmjs.org/:_authToken xxxx-xxxx-xxxx-xxxx-xxxxxxxxx

I expect to be able to be able to access the private packages on the registry.

I believe this relates to #3932 as well as a few others.

Explanation: YarnRegistry extends NpmRegistry however NpmRegistry.getScopedOption does not try to read from YarnRegistry.config so it is unable to find //registry.npmjs.org/:_authToken resulting in errors.

otaku · 2018-05-12T20:52:41Z

src/registries/npm-registry.js

@@ -298,8 +298,7 @@ export default class NpmRegistry extends Registry {
    }

    for (const scope of [this.getScope(packageIdent), '']) {
-      const registry =
-        this.getScopedOption(scope, 'registry') || this.registries.yarn.getScopedOption(scope, 'registry');


YarnRegistry.getScopedOption does not exist.

BYK · 2018-05-13T17:51:47Z

@arcanis @rally25rs @imsnif can you take a look?

@otaku thanks a lot for the fix! Do you think you can add tests for the fix?

imsnif

Hey @otaku - thanks a lot for this patch! I left one comment. To elaborate: if I understand correctly, we could achieve the same effect by implementing YarnRegistry.getScopedOption and leaving the registry assignment in the for loop as is. Or am I missing something? What do you think?
Also, a test case for this would definitely be great.

Other than that, looks like an important fix. I feel like I've encountered this issue in the past and had to dance around it without looking too much into it. :)
Would be happy to merge. Good find!

imsnif · 2018-05-13T18:44:40Z

src/registries/npm-registry.js

+      }
+    }
+
+    return val;


Do you think it will be possible to swap this for an implementation of YarnRegistry.getScopedOption?

I believe that would turn into endless recursion.

NpmRegistry.getScopedOption would loop through all registries looking for getScopedOption and call it, however if it doesn't exist in that registry, it in turn would do the same thing. It could be possible by passing optional arguments to not recurse... but I don't know if that's worth the complexity.

Sorry, I'll clarify my meaning. How aboutif we implement YarnRegistry.getScopedOption as something like:

getScopedOption(scope: string, option: string): mixed { return this.getOption(scope + (scope ? ':' : '') + option); }

YarnRegistry is an extension of NpmRegistry so getScopedOption is the same function as this one.

I could look into moving all the logic into BaseRegistry and remove them from both YarnRegistry and NpmRegistry if that's the desired goal?

I think we can start by adding a test for this and then playing around to see what works best. It'll be much easier that way. Makes sense?

otaku · 2018-05-13T22:22:34Z

I've added a simple test case against two scoped registries. Let me know if there's any more changes needed. I originally attempted to modify all test cases to run with YarnRegistry but that quickly became a headache.

I don't understand in which scenarios https://registry.yarnpkg.com should receive the NPM authentication token; or why registry.yarnpkg.com is even receiving authTokens for registry.npmjs.org. I believe it was introduced in #5216

All the test cases appear to be for NpmRegistry and none exist for YarnRegistry

otaku · 2018-05-14T05:15:55Z

Alternatively, instead of YarnRegistry checking NpmRegistry and NpmRegistry checking YarnRegistry.

I've modified BaseRegistry.getOption to include an optional argument boolean searchRegistries with a default value of true which will search other registries for the same key. The searchRegistries argument so other registries may call super.getOption. I've also moved the npmMap and turned into aliases in BaseRegistry.

This is a slightly cleaner implementation as it moves everything into BaseRegistry.

Tests are passing as well. Check it out here: otaku/yarn@master...otaku:registry-fix2

Let me know if I should merge these changes into this PR.

Also, to attempt to answer your first question / concern:

yarn/src/fetchers/tarball-fetcher.js

Line 151 in ec8cea0

const registry = this.config.registries[this.registry];

registry is set to npm. It directly calls NpmRegistry.request bypassing YarnRegistry entirely. It never actually tries to get the configuration option from YarnRegistry config anywhere.

imsnif · 2018-05-14T10:40:11Z

Hey @otaku - first: thanks for all the rapid work on this!

I reverted your src changes locally and the new test-case you added still passes. A brief look tells me maybe it needs to be adjusted a little? Feel like taking a look?
Once we have a test that consistently fails without the changes and passes with them, it'll be significantly easier to decide where these changes should live (base, YarnRegistry, NpmRegistry, etc.)

As for tests - there indeed don't seem to be unit tests for YarnRegistry. While it is tested in the integration tests (and also in some other tests), I agree that it's Not Good(tm). iirc we plan on merging the two paths soon, so that will solve this particular issue.

arcanis · 2018-05-14T10:47:44Z

Just want to mention that the BaseRegistry / YarnRegistry / NpmRegistry classes are a nonsense that we want to eventually merge into a single implementation. Just make sure that any change you make there won't make this more difficult 🙂

otaku · 2018-05-14T14:05:13Z

Fixed the test case. I had it running against YarnRegistry instead of NpmRegistry. Given that you're moving to a single implementation, I went ahead and merged my other changes in.

imsnif · 2018-05-15T07:48:36Z

@otaku - let me know if you want a hand fixing those tests.

otaku · 2018-05-15T12:18:01Z

All good to go now! The recursive getOption broke everything.

imsnif · 2018-05-16T15:13:13Z

Hey @otaku - thanks again for your work on this.

The issue I find with this implementation (and please correct me if I missed something) is that it causes .yarnrc and .npmrc to be merged in a rather confusing way. Essentially, if I have a config key in one of them, it will be used.
Sometimes (like in the case of the registry here) .npmrc will take precedence, and at other times .yarnrc will take precedence (I think in most if not all other cases).
It's confusing to us maintainers and thus it will certainly be confusing for users.
I tried to find an easy way to fix this (starting out in my initial comments about implementing getScopedOption in the YarnRegistry), but I think it does not make sense the way things are in the registries right now.

I feel it would be much better, maintainability-wise and usability-wise to wait for the bigger refactor @arcanis talked about above. Where we can decide how the two config files are merged exactly and write it out explicitly. Barring that, I think a temporary solution that would simplify rather than add complexity is addressing this issue: #3683
Doing that would also bring us a step closer to merging these three classes. If you'd want to go in that direction and implement it yourself, I'd be happy to review and merge that in a separate PR. What do you think? Makes sense?

otaku · 2018-05-16T16:30:51Z

@imsnif You're not wrong. It's actually confusing even trying to follow the code. Currently, you have resolvers which figures out which registry to read from. Given the registry that it choice, the following occurs:

Currently in `master`:

NPMRegistry
- .npmrc
YarnRegistry:
- .yarnrc
- aliases from .npmrc
- .npmrc
- defaults (an object)

This PR:

NPMRegistry
- .npmrc
- .yarnrc
- Aliases, following above order
YarnRegistry:
- .yarnrc
- .npmrc
- Aliases, following above order
- defaults (an object)

This is the temporary solution that I came up with for now, otherwise I could change this PR into a larger refactor of one registry with multiple configuration sources.

If you'd like a PR for the larger refactor I need a few answers:

How does yarnpkg.com registry work?
When should yarnpkg.com receive registry.npmjs.org tokens?
Why not use config settings to set a registry for certain packages so that it will directly use registry.npmjs.org and not have to pass tokens to yarnpkg.com? (assuming yarnpkg.com is a proxy)
What's the actual priority of config files?
How should the configuration be merged together?

imsnif · 2018-05-16T19:23:13Z

Hey @otaku - great! So, let's get things rolling.

Since we're changing the behaviour of the config files (what we call fixing a bug might be called "creating a bug" by someone else) - I wouldn't want to do it more than we absolutely have to. So ideally, I'd want to do it just once.

I think the desired precedence for config files is:

.yarnrc
.npmrc
defaults*

IMO, this can be achieved as mentioned here: #3683 by extracting the loadConfig method into the BaseRegistry and placing all that logic there (not in one function ofc - but you get my drift :) ). Then all registries will have the same config and this problem will go away. Also, we'll be much closer to merging these classes.
What do you think?

*I'm ignoring the aliases right now for simplicity.

imsnif · 2018-05-16T19:24:39Z

Do note that if we go this way, I think we'll need to add quite a lot of new test cases that will articulate this behaviour - because as far as I saw this is hardly tested right now.

imsnif · 2018-05-25T14:08:53Z

Hey, @otaku - do you feel like picking this up?

otaku · 2018-05-25T14:43:16Z

I currently do not have time due to work @imsnif

imsnif · 2018-05-25T14:52:26Z

Thanks for your work and effort up until now @otaku!

tregusti · 2018-08-08T06:39:10Z

Was this added/fixed in some other PR? I would like to use it =)

jonaskello · 2018-08-28T14:58:37Z

This would be useful to me too, was it ever merged?

codepunkt · 2019-02-27T15:49:25Z

Same here. I find the combination of NPMRegistry and YarnRegistry and their handling of .npmrc/.yarnrc a major source of confusion. Either document the exact behavior, including a description of the methods using .npmrc information and those using .yarnrc information, or do something along the lines of this pull request.

It's literally impossible to configure yarn to talk to an internal private registry (including authentication and over SSL) simply by reading the docs. I have no clue what information goes where, all i know is that information configured in .yarnrc is not read in cases where it should be in order to work.

I cannot possibly know which of these configuration tidbits (registry, _auth, strict-ssl, always-auth and more) has to be in which configuration file. Factor in a mix of local (project) and global (user dir) configuration and i'm basically screwed.

Help.

/cc @imsnif @arcanis

arcanis · 2019-02-27T16:02:19Z

The system is completely overhauled in the v2. Yarn will only read .yarnrc files, and the configuration settings can be queried via yarn config -v for an easier discoverability. Contributions welcome to move faster towards a release!

Attempt to get config option from YarnRegistry

b88afa4

otaku commented May 12, 2018

View reviewed changes

otaku force-pushed the master branch 2 times, most recently from 7f44bca to ced8487 Compare May 12, 2018 21:26

If unable to get option attempt to read option from other registries

4df15f9

otaku force-pushed the master branch from ced8487 to 4df15f9 Compare May 12, 2018 21:27

imsnif requested changes May 13, 2018

View reviewed changes

imsnif self-assigned this May 13, 2018

Add test case for YarnRegistry config storing authentication

b79101f

Correct test case for YarnRegistry

acc231c

otaku force-pushed the master branch from 1c126d6 to 0b0e97a Compare May 14, 2018 14:09

Simplify Registries

04cbc49

otaku force-pushed the master branch from 0b0e97a to 04cbc49 Compare May 14, 2018 14:14

remove recursiveness, directly access config

6a83cea

BYK mentioned this pull request May 23, 2018

fix(config): allow .npmrc config to supersede yarn defaults #5825

Closed

imsnif closed this May 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read config from ~/.yarnrc #5812

Read config from ~/.yarnrc #5812

otaku commented May 12, 2018

otaku May 12, 2018 •

edited

Loading

BYK commented May 13, 2018

imsnif left a comment

imsnif May 13, 2018

otaku May 13, 2018 •

edited

Loading

imsnif May 13, 2018

otaku May 13, 2018 •

edited

Loading

imsnif May 13, 2018

otaku commented May 13, 2018 •

edited

Loading

otaku commented May 14, 2018 •

edited

Loading

imsnif commented May 14, 2018

arcanis commented May 14, 2018

otaku commented May 14, 2018

imsnif commented May 15, 2018

otaku commented May 15, 2018

imsnif commented May 16, 2018

otaku commented May 16, 2018

imsnif commented May 16, 2018 •

edited

Loading

imsnif commented May 16, 2018

imsnif commented May 25, 2018

otaku commented May 25, 2018

imsnif commented May 25, 2018

tregusti commented Aug 8, 2018

jonaskello commented Aug 28, 2018

codepunkt commented Feb 27, 2019 •

edited

Loading

arcanis commented Feb 27, 2019

Read config from ~/.yarnrc #5812

Read config from ~/.yarnrc #5812

Conversation

otaku commented May 12, 2018

otaku May 12, 2018 • edited Loading

Choose a reason for hiding this comment

BYK commented May 13, 2018

imsnif left a comment

Choose a reason for hiding this comment

imsnif May 13, 2018

Choose a reason for hiding this comment

otaku May 13, 2018 • edited Loading

Choose a reason for hiding this comment

imsnif May 13, 2018

Choose a reason for hiding this comment

otaku May 13, 2018 • edited Loading

Choose a reason for hiding this comment

imsnif May 13, 2018

Choose a reason for hiding this comment

otaku commented May 13, 2018 • edited Loading

otaku commented May 14, 2018 • edited Loading

imsnif commented May 14, 2018

arcanis commented May 14, 2018

otaku commented May 14, 2018

imsnif commented May 15, 2018

otaku commented May 15, 2018

imsnif commented May 16, 2018

otaku commented May 16, 2018

Currently in master:

This PR:

imsnif commented May 16, 2018 • edited Loading

imsnif commented May 16, 2018

imsnif commented May 25, 2018

otaku commented May 25, 2018

imsnif commented May 25, 2018

tregusti commented Aug 8, 2018

jonaskello commented Aug 28, 2018

codepunkt commented Feb 27, 2019 • edited Loading

arcanis commented Feb 27, 2019

otaku May 12, 2018 •

edited

Loading

otaku May 13, 2018 •

edited

Loading

otaku May 13, 2018 •

edited

Loading

otaku commented May 13, 2018 •

edited

Loading

otaku commented May 14, 2018 •

edited

Loading

Currently in `master`:

imsnif commented May 16, 2018 •

edited

Loading

codepunkt commented Feb 27, 2019 •

edited

Loading