Shallow merge top-level object parameters

[shilman]

Currently, the options and docs parameters are shallow merged. I propose applying this behavior for all object parameters, as a breaking change in Storybook 6.0.

Here's the reference code: https://github.com/storybookjs/storybook/blob/next/lib/client-api/src/client_api.ts#L93-L105

Motivation

The motivation for this is purely ergonomic.

In addon-docs, we set some configurations in the preset, and then users can add/override configurations on a per-story basis.

Applying this behavior to all object parameters would mean we don't have to special-case docs inside core. It would also be useful in other cases, like making it easy to set the default background on a per-story basis: #9164

cc @ndelangen @tmeasday

[kevin940726]

I don't think it would work in the context of #9164 (I just noticed that you've explicitly mentioned that it won't solve that issue 😅). Even though the merging algo will concat the array without duplicates, it would not work when we cannot effectively identify duplications.

// The base
backgrounds: [
  { name: "white", value: "#ffffff", default: true },
  { name: "black", value: "#000000" }
];

// Overide
backgrounds: [{ name: "black", value: "#000000", default: true }];

Since the new entry is not equal to any of the entries in the original array, it would just get appended to the end instead of de-dup the black entry in the base.

We could special-case name or id as an identification to the array, so that the merging algo could de-dup based on the id. However, I think that it over-complicates things and makes things hard to predict and error-prone.

How about accepting functions as parameter values?

// Overide
backgrounds: originalArray => {
  const newArray = originalArray.map(item => ({ ...item }));
  // do something with newArray
  return newArray;
};

This way, we could provide maximum flexibility without having to do some complicated merging algos. We could still provide the default merging algo if the value is not a function. The downside of this is that if the parameter itself accepts a function then we would have to make it into higher-order functions, I wonder if that's case for any addons now?

[tmeasday]

This proposal is pretty limited and I would say uncontroversial (how often are people calling addParameters({ key }) at the top-level more than once?). 6.0 is a great time to do it, given it's technically breaking.

It would also be useful in other cases, like making it easy to set the default background on a per-story basis: #9164

I don't think so:

• If you mean what I think you mean (what happens when you call addParameters twice globally), it won't help with that use case (wanting to do something per-story).

• If you mean extending this behaviour to component-level and story-level parameters, we already do that, and more:

storybook/lib/client-api/src/client_api.ts

Lines 243 to 261 in a1a1cdd

	const allParam = [_globalParameters, localParameters, parameters].reduce(
	(acc: Parameters, p) => {
	if (p) {
	Object.entries(p).forEach(([key, value]) => {
	const existingValue = acc[key];
	if (Array.isArray(value)) {
	acc[key] = value;
	} else if (isPlainObject(value) && isPlainObject(existingValue)) {
	acc[key] = merge(existingValue, value);
	} else {
	acc[key] = value;
	}
	});
	}
	return acc;
	},
	{ fileName }
	);

But it doesn't help in this case where the backgrounds parameters is a plain array. That's why we recommend that parameters should always be an object, but anyway, 🤷‍♂ .

I suspect a better solution to the larger problem might be to have both parameters and unmergedParameters on the context. unmergedParameters could look like:

context = {
  unmergedParameters: {
    global: { backgrounds: {...} },
    component: { backgrounds: {...} },
    story: { backgrounds: {...} },
  }
}

We could even make global an array to reflect the fact that it can be called more than once (in CSF the others cannot).

That way an addon can look at parameters (the automerged thing) if it wants the current behaviour and unmergedParameters otherwise.

Funnily enough we ran into this exact problem with Chromatic a day or two ago: chromaui/chromatic-cli#80 (cc @kylesuss)

[tmeasday]

Note that the larger change would also make sense to only pass global parameters over the channel once, rather once than per-story (and similar with component parameters).

[shilman]

@tmeasday @kevin940726 Ok I didn't realize we were already applying merge to global and local parameters. In that case, my change is pretty uncontroversial. unmergedParameters also seems pretty uncontroversial to me, tho I wonder whether it could be abused or add bloat.

As for backgrounds, I guess it's unrelated, so I posted separately there: #9164 (comment)

[stale]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

[shilman]

Duplicate #7872