adds optional options argument; adds deepmerge to options

[apeltz]

No description provided.

[apeltz]

@developit @pl12133 this may be a solution looking for a problem, but addresses conflicts in which a non-primative prop is passed to a consuming component by multiple wrappers, and the consuming component would like the resulting prop to be a merged version

[pl12133]

@apeltz Totally forgot about this, I can review tomorrow. This is definitely an issue which creates very unexpected behavior so I might even prefer skipping options and doing a deep merge by default.

[pl12133]

To summarize the problem here:

You create a withConfig enhancer using defaults, with nested objects:

const defaultConfig = {
  fooConfig: {
    defaultKey: 'defaultValue'
  }
};

const withConfig = preconf(null, defaultConfig);

So far so good, your Component gets an Object from @withConfig('fooConfig') shaped as { defaultKey: 'defaultValue' }.

The problem surfaces when a component somewhere else in the tree adds additional keys to the nested object:

const overrideConfig = {
  fooConfig: {
    overrideKey: 'overrideValue'
  }
}
const withConfig= preconf(null, overrideConfig);

Now, you may be expecting that @withConfig('fooConfig') will return an Object shaped like this:

{
  overrideKey: 'overrideValue',
  defaultKey: 'defaultValue'
}

However by adding fooConfig to context, the value of fooConfig is now pulled from context.config instead of defaults. The effectively means that the overrideConfig has replaced instead of merged with the default values from defaultConfig. Therefore @withConfig('fooConfig') will return an Object with one key, { overrideKey: 'overrideValue' }.

As far as default behavior goes, it's extremely confusing. 👻 Rather than adding an option to disable it, I think we should probably move forward with a merge being the only behavior.

[apeltz]

I forgot about this until now. Is this something we still think is useful? @pl12133 @Aneurysm9

[pl12133]

Yup, we should definitely get this merged. I'm thinking mostly we just need to test it on some existing components and see if it breaks anything.

[developit]

Agreed with @pl12133 - the current behaviour was never meant to handle returning merged objects, only leaf values.

[apeltz]

It should be merged as a major semver update since it's breaking anyway. Then we can selectively update components to validate that nothing is broken or patch what is?

[pl12133]

Interestingly @billneff79 might have found a way to solve this at the <Provider> level: synacor/preact-context-provider#3.

[pl12133]

It runs, let's ship it as a beta! Thanks for working on this @apeltz

[apeltz]

@pl12133 some updates in this latest commit:

• made the naming more similar to @billneff79 's addition to preact-context-provider
• added the ability to accept arrays of keys to selectively deep-merge
• added a option to toggle the merging precedence between the node config and config from contex, defaulted to context overriding default/node config

[apeltz]

@pl12133 I added some test cases that are more readable and also fixed an error in a docblock example use case I had.

@billneff79 's synacor/preact-context-provider#3 does provide a path to accomplish the same thing that the functionality here does, but this addition will allow us to continue to use preconf as we currently are while without an additional provider wrapper.

[billneff79]

I believe this solves a similar, but different problem to synacor/preact-context-provider#3. preact-context-provider solves the merging issue when you are using <Provider config={}> at multiple levels in the tree. In order to have preconf play nicely with that, I think we have two options:

1. Merge this PR, which makes the defaults that preconf provides act exactly as though you had wrapper the module in a <Provider config={defaults}> tag
2. Modify preconf to actually wrap the component in a <Provider config={defaults}> tag when you apply the @withConfig decorator, so we don't make code that is redundant to Provider.
3. Remove the ability for preconf to specify defaults at all and change the expectation to one where we expect you to provide defaults to your component through a <Provider> explicitly near the top of your app/component heirarchy.

Part of me leans toward #3 as I find the default functionality redundant with the new capabilities provided by <Provider>.

[apeltz]

Going to close this PR after discussions with @billneff79 resulted in a new approach.

[billneff79]

To be clear, I think our new approach will be to take the functionality that preconf provides and move it more generically into a new function in synacor/preact-context-provider as we've found uses, beyond config, where we'd like to be able to cherry pick values out of objects in context and provide them as props.