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.

Sign up for GitHub

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

Jump to bottom

Read config from ~/.yarnrc #5812

Closed
wants to merge 6 commits into from
Closed

Read config from ~/.yarnrc #5812

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b88afa4
Attempt to get config option from YarnRegistry
otaku May 12, 2018
4df15f9
If unable to get option attempt to read option from other registries
otaku May 12, 2018
b79101f
Add test case for YarnRegistry config storing authentication
otaku May 13, 2018
acc231c
Correct test case for YarnRegistry
otaku May 14, 2018
04cbc49
Simplify Registries
otaku May 14, 2018
6a83cea
remove recursiveness, directly access config
otaku May 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 19 additions & 3 deletions src/registries/npm-registry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Author

@otaku otaku May 12, 2018 •
edited

Choose a reason for hiding this comment

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

YarnRegistry.getScopedOption does not exist.

const registry = this.getScopedOption(scope, 'registry');
if (registry) {
return String(registry);
}
Expand Down Expand Up @@ -347,7 +346,24 @@ export default class NpmRegistry extends Registry {
}

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

let val = this.getOption(key);

if (typeof val === 'undefined') {
const registries = Object.keys(this.registries);
for (const registryName of registries) {
if (this.registries[registryName].getOption) {
val = this.registries[registryName].getOption(key);
}

if (typeof val !== 'undefined') {
break;
}
}
}

return val;
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 it will be possible to swap this for an implementation of YarnRegistry.getScopedOption?

Copy link
Author

@otaku otaku May 13, 2018 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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);
}

Copy link
Author

@otaku otaku May 13, 2018 •
edited

Choose a reason for hiding this comment

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

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?

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 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?

}

getRegistryOption(registry: string, option: string): mixed {
Expand Down