Core: Story context is prepared before for supporting fine grained updates

[webblocksapp]

Issue: #20584

What I did

I needed to make some changes to how we prepare stories, in order to make args reactive in frameworks that use reactive proxies such as SolidJS or Vue for fine-grained updates.

This change modifies the way Storybook currently prepares stories which causes reactive args to get lost for fine-grained updates JS frameworks. That's because for those frameworks handle args/props as proxies behind the scenes to make reactivity work. So when we do the argType mapping in prepareStory the Proxies are destroyed and args becomes a plain object again, losing the reactivity.

The proposal here is to change the flow, passing the mapped args at renderToCanvas so that the proxies stay intact.

There's a chance that this change will also make reactive args in Vue3 possible, something that we currently can't do without re-creating the whole Vue story.

Note
This change means that args will be mapped prior to being passed to decorators, where previously decorators received raw args, they now receive mapped args. This is the breaking change.

How to test

1. Run a sandbox for template, e.g. yarn task --task sandbox --start-from auto --template react-vite/default-ts
2. Open Storybook in your browser
3. Access X story

• Unit tests:

prepareStory.test.ts
StoryRender.test.ts

@tmeasday - It was added a test at prepareStory.test.ts to confirm that prepared args are received on each decorator.

Checklist

• Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
• Make sure to add/update documentation regarding your changes
• If you are deprecating/removing a feature, make sure to update
  MIGRATION.MD

Maintainers

• If this PR should be tested against many or all sandboxes,
  make sure to add the ci:merged or ci:daily GH label to it.
• Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

[JReinhold]

I don't know why snapshots didn't report for you, I just did yarn test StoryStore -u and it updated the snapshots. 🤷

[JReinhold]

@webblocksapp could you add an entry to the MIGRATION.md under https://github.com/storybookjs/storybook/blob/next/MIGRATION.md#70-breaking-changes that explains that decorators will now received mapped args, where previously they received raw args and only the story received the mapped args?

[webblocksapp]

@JReinhold updated MIGRATION.md

[webblocksapp]

I don't know why snapshots didn't report for you, I just did yarn test StoryStore -u and it updated the snapshots. 🤷

Thxs for the clarification, it was my bad. I was running -u in the wrong test.

[tmeasday]

We'll also need to update this function to prepare the context:

storybook/code/lib/preview-api/src/modules/store/csf/testing-utils/index.ts

Lines 75 to 84 in 2cceaad

	const composedStory = (extraArgs: Partial<TArgs>) => {
	const context: Partial<StoryContext> = {
	...story,
	hooks: new HooksContext(),
	globals: defaultGlobals,
	args: { ...story.initialArgs, ...extraArgs },
	};
	return story.unboundStoryFn(context as StoryContext);
	};

[tmeasday]

@webblocksapp thanks very much, can I ask for one more thing -- now the arg mapping crosses the module boundary between prepareStory and StoryRender I think we need an integration test. Can I get you to add one to PreviewWeb.test.ts?

Maybe we can simply modify this test to add a mapping?

storybook/code/lib/preview-api/src/modules/preview-web/PreviewWeb.test.ts

Line 461 in 36e7a95

it('passes loaded context to renderToCanvas', async () => {

[webblocksapp]

@tmeasday - Added integration tests. This modification affected an average of 34 tests at PreviewWeb.test.ts when adding a second arg with mapped argTypes to the mock.

[JReinhold]

It looks like the e2e tests for Actions are failing in this PR. I've investigated a bit and confirmed locally that the button indeed does not log any actions when clicked, which it should. I'll continue investigating where it breaks when I can.

[tmeasday]

Thanks for tweaking the tests!

[webblocksapp]

It looks like the e2e tests for Actions are failing in this PR. I've investigated a bit and confirmed locally that the button indeed does not log any actions when clicked, which it should. I'll continue investigating where it breaks when I can.

@JReinhold confirmed, actions are broken. Inspecting the console gives the following error when clicking the blue button inside the button story.

react-dom.development.js:4312 Uncaught TypeError: Converting circular structure to JSON
    --> starting at object with constructor 'HTMLDivElement'
    |     property '__reactContainer$m7ddawai0b' -> object with constructor 'FiberNode'
    |     property 'stateNode' -> object with constructor 'FiberRootNode'
    |     ...
    |     property 'pendingProps' -> object with constructor 'Object'
    --- property 'canvasElement' closes the circle
    at JSON.stringify (<anonymous>)
    at stringify (index.mjs:1453:15)
    at PostmsgTransport.send (index.mjs:71:18)
    at handler (index.mjs:32:24)
    at Channel.emit (index.mjs:39:7)
    at onClick (chunk-UYZOVUF7.mjs:68:13)
    at index.js:449:39
    at index.js:457:11
    at mockConstructor (index.js:170:19)
    at HTMLUnknownElement.callCallback2 (react-dom.development.js:4164:14)

Let me know if you have any clue about this. From my side, tomorrow I will also continue checking the issue.

[JReinhold]

I tracked down the issue, and pushed a fixed (at least it worked locally).

Basically, when an action event is triggered, it is serialized (stringified) so it can be emitted over the channel. Stringification breaks if there are circular structures, which is what happened here.

There has always been a circular structure in React Synthetic events, however our stringifier defaults to stop stringifying when it reaches a depth 15.
For some reason, this depth is now too high (low?) for JSON.stringify to ignore the circular structure, so it crashes.

The highest depth that can now be used while still not crashing is depth 11, which I've changed the default to (it's minDepth (5) + default (6)). This is plenty deep for the events to still be valuable, so I don't think it's a problem to change it.

However why this PR requires the change, I don't know. My only idea is that maybe our previous arg mapping would make the event uncircular because of how we did object assigning, and now that the args are more "pure", the circular structure remains at the time of serialization. Maybe not, maybe @tmeasday knows.

[JReinhold]

💪

[tmeasday]

@JReinhold is the circular object in the args? Looking at the preview-web tests we don't ever send the mapped args over the channel (good!), so it would be surprising if so.

If not, it's weird that this issue has just started in this PR.

[webblocksapp]

I was checking @JReinhold changes, so the issue was at groupArgsByTarget because of the empty string used as a key for groupedArgs. I'm curious: what this function does and also what was the issue with the empty string?

[tmeasday]

I was checking @JReinhold changes, so the issue was at groupArgsByTarget because of the empty string used as a key for groupedArgs

Yeah, that's right. It is a (edge casey) bug in telejson: storybookjs/telejson#93

It sort of makes sense why this PR triggered it, but we didn't trace down the exact reasons. Basically the argsByTarget object being added to the context at an earlier point ended up putting it as a prop on a higher dom element in the event.

I'm curious: what this function does and also what was the issue with the empty string?

Targetted args is an undocumented feature that we added to allow experimentation. We haven't really developed the concept yet but you can read about it here: https://chromatic-ui.notion.site/Args-for-side-loaded-data-6ba311ecc1c7447197e0cbf404dd3dbf

The choice to use '' as the key for the untargetted args was kind of a weird one.

[webblocksapp]

It's awesome! I'm excited to see this PR was merged. Thxs a lot for your great support to help me make this contribution a reality. It was a pleasure to work with you @JReinhold, @tmeasday and the Storybook team. With this, I will start next week working on the organization repo with the SolidJS renderer and framework.

[JReinhold]

Great!

I'm eager to assist you with anything you need going forward, let's make this a kick-ass integration.

I'll close the feedback PR now.