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

Use Object.assign over defaults. #2687

Merged
merged 1 commit into from
Feb 27, 2017
Merged

Use Object.assign over defaults. #2687

merged 1 commit into from
Feb 27, 2017

Conversation

wtgtybhertgeghgtwtg
Copy link
Contributor

Summary
Drops the defaults package and uses Object.assign. While

this.config = Object.assign({}, DEFAULTS, this.config);

is a little less obvious than

defaults(this.config, DEFAULTS);

I thought it might be worth making a PR.

Test plan
Since it's an internal, no tests have been added that strictly test this.

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

Travis failing because ember-cli is not resolving.

@@ -139,7 +138,7 @@ export default class NpmRegistry extends Registry {
await fs.mkdirp(mirrorLoc);
}

defaults(this.config, config);
this.config = Object.assign({}, config, this.config);
Copy link
Member

Choose a reason for hiding this comment

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

I understand that defaults mutates the object instead of creating a copy.
Would it be a problem when passing a particular property across modules?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might if you're sharing a config object between registries and relying on outside mutations.

Copy link
Member

Choose a reason for hiding this comment

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

Could you check if this is the case for the options added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it is. this.config is initialized as an empty object when a registry is constructed, and its values are loaded from a .ini or a lockfile.
The only case I came come up with is if you have something like

const yarnReg = new YarnRegistry();
await yarnReg.loadConfig();
const config = yarnReg.config.
await yarnReg.loadConfig();
// config and yarnReg.config will be different and may have different values if the lockfile has been edited between `yarReg.loadConfig` calls.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for looking into this, @wtgtybhertgeghgtwtg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for merging.

@bestander
Copy link
Member

Try rebasing to fix CI test

@bestander bestander merged commit f8fd813 into yarnpkg:master Feb 27, 2017
@wtgtybhertgeghgtwtg wtgtybhertgeghgtwtg deleted the drop-defaults branch February 27, 2017 11:33
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