Skip to content
This repository has been archived by the owner on Oct 6, 2021. It is now read-only.

Support default settings #176

Merged
merged 2 commits into from
Oct 4, 2017
Merged

Conversation

ljpengelen
Copy link
Contributor

No description provided.

const envSpecificSettings = require(`./settings.${process.env.NODE_ENV}.json`);
const defaults = require("./settings.default.json");

export default Object.assign(defaults, envSpecificSettings);
Copy link
Member

Choose a reason for hiding this comment

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

Object.assign is modifying defaults here. I'm not entirely sure but my guess is it would actually globally modify the value of require("./settings.default.json")) since export statements are only evaluated once.

So to be on the safe side I think it would be better to make this Object.assign({}, defaults, envSpecificSettings)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, this has an unintended side effect. I didn't realize that require("./settings.default.json") in one file returns the same object as require("./settings.default.json") in another file. I'll commit an updated version of this file.

@pascalw pascalw merged commit f8750b5 into master Oct 4, 2017
@pascalw pascalw deleted the feature/support-default-settings branch October 4, 2017 10:12
@pascalw
Copy link
Member

pascalw commented Oct 4, 2017

Thanks 👍

@pascalw
Copy link
Member

pascalw commented Oct 4, 2017

I forgot to mention that the CHANGELOG should also be updated. I've done this now for you on master :-)

@pascalw pascalw mentioned this pull request Oct 4, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants