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

Ability to set default options #89

Closed
adamreisnz opened this issue Dec 13, 2017 · 6 comments
Closed

Ability to set default options #89

adamreisnz opened this issue Dec 13, 2017 · 6 comments

Comments

@adamreisnz
Copy link

adamreisnz commented Dec 13, 2017

Would you be open for a PR where we can specify the default options globally?

My arrayMerge strategy is to always overwrite the array instead of concatenating, so it's quite annoying to have to specify the options with the overwrite function every time I use deepmerge across various files, and it has already caused some annoying bugs.

Example:

const merge = require('deepmerge');
merge.setDefaultOptions({
  arrayMerge: (dest, source) => source,
});

Afterwards, every merge would use those options as the default, unless specific options are passed in.
Happy to create a PR for it.

@macdja38
Copy link
Collaborator

hmm. it wouldn't be a good idea to have that change the global deepmerge that's being required, so it would probably be best if menge.setDefaultOptions returns a copy of merge that can be used instead... something like

const merge = require('deepmerge').setDefaultOptions({
  arrayMerge: (dest, source) => source,
});

Otherwise two libraries in the same application that use deepmerge could interfere.

@adamreisnz
Copy link
Author

Yes, I was planning to make it chainable and return a copy of merge

@TehShrike
Copy link
Owner

Either way, you still have to do the same thing in your project - which is make a merge.js file in your shared/common folder for everything in your project to import.

I'm hesitant to add new code to this project, since setting defaults on your own is so easy to do, and there are even different flavors you might want to expose internally.

Able to override options:

import arrayMerge from './somewhere'
import deepmerge from 'deepmerge'
export default (a, b, options) => deepmerge(a, b, Object.assign({ arrayMerge }, options))

Unable to override options:

import arrayMerge from './somewhere'
import deepmerge from 'deepmerge'
export default (a, b) => deepmerge(a, b, { arrayMerge })

Able to merge any number of objects:

import arrayMerge from './somewhere'
import deepmerge from 'deepmerge'
export default (...objects) => deepmerge.all(objects, { arrayMerge })

I think it's better to let everyone decide how they want merge to be exposed to their project with a line of JS instead of using a baked-in extend function on the library.

@adamreisnz
Copy link
Author

adamreisnz commented Dec 16, 2017

@TehShrike no, that's incorrect. I am using ES6 and standard require's, as I imagine the majority of people using Node still do.

As such, all require's are cached, so I can simply set the defaults once, in my initialisation file, and then whenever I require deepmerge elsewhere in my application I won't have to worry about the defaults.

E.g.:

init.js

//Set default merge options for deep merge
require('deepmerge').setDefaultOptions({
  arrayMerge: (dest, source) => source,
});

anywhere-else.js

const merge = require('deepmerge');
merge(a, b); //Will use the defaults as defined earlier.

The thing is, there must already be defaults (e.g. the default array merge is to concatenate), so I simply want an interface to be able to modify those defaults. The current approach is too cumbersome to manage efficiently across a large codebase.

@TehShrike
Copy link
Owner

Yes, that's what @macdja38 was addressing - that syntax also affects any other modules that you're requiring. If you're using some gulp library or whatever that uses deepmerge, you don't want to mess with its default properties.

Your usage of deepmerge shouldn't affect some webpack library that also happens to use deepmerge.

Ignore the ES Module syntax in my previous response. The problems are exactly the same even if I use CommonJS in the examples.

@adamreisnz
Copy link
Author

adamreisnz commented Dec 16, 2017 via email

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

No branches or pull requests

3 participants