Core: Normalize parameters in store/channel #10373

tmeasday · 2020-04-10T01:26:02Z

WIP:

Store parameters in the store normalized
- Update add/tests
Emit stores over channel normalized
- Figure out format
- Update stories module to combine them into storiesHash
- Update refs module to do the same, but back-compatible with old format.
Migration guide
- Make sure combineParameters is available.

github-actions · 2020-04-10T01:28:09Z

	Fails
🚫	PR is marked with "BREAKING CHANGE" label.

Generated by 🚫 dangerJS against e2d54f6

lib/client-api/src/story_store.ts

…eters

@shilman

Is this bad @shilman?

Because circular deps

tmeasday · 2020-04-22T13:59:14Z

@shilman @ndelangen can you take a look? This is mostly done but there are a few things in here that may be controversial:

New event instead of changing the data to SET_STORIES. It seems like this is less likely to cause subtle bugs. Plus it makes it easier to support legacy events in refs.
A second implementation of combineParameters. I feel like we should have some raw "datatypes" package that we can depend on everywhere (that would include types[1]). Potentially that package is csf or something like that.
Other stuff?

[1] There is heaps of inconsistent duplication of types in the code base it is a mess.

tmeasday · 2020-04-22T14:00:35Z

lib/api/src/tests/stories.test.js

+      const fullAPI = { on: jest.fn(), setStories: jest.fn(), setOptions: jest.fn() };
+      const navigate = jest.fn();
+      const store = createMockStore();
+
+      const { init } = initStories({ store, navigate, provider, fullAPI });
+      init();
+
+      const onSetStoryStoreData = fullAPI.on.mock.calls.find(
+        ([event]) => event === SET_STORY_STORE_DATA
+      )[1];


Is there a better way to do this for tests @ndelangen?

lib/client-api/src/story_store.ts

tmeasday · 2020-04-22T14:03:24Z

lib/client-api/src/story_store.ts

@@ -401,6 +405,7 @@ export default class StoryStore {

      return {
        ...data,
+        parameters: this.combineStoryParameters(data.parameters, data.kind),


It wasn't exactly clear which APIs we should produce denormed/combined parameters for on the story store. Right now it may be very confusing which ones we do it for.

shilman

I agree about the need for a separate package to dedupe a small set of core data structures and convenience functions

lib/client-api/src/story_store.ts

ndelangen · 2020-04-23T08:29:42Z

@tmeasday the set_stories event's data is currently something like { stories: {...} }.

I made it that way so we could add more data to it in the future!

I think we don't want an extra event, that would probably complicate things more? More async events, race-conditions and stuff?

# Conflicts: # lib/api/src/modules/stories.ts

tmeasday · 2020-04-24T10:21:25Z

I think we don't want an extra event, that would probably complicate things more? More async events, race-conditions and stuff?

I wasn't going to add an extra event, just replace the existing SET_STORIES with SET_STORY_STORE_DATA.

I've discussed this with both you and @shilman and the conclusion we reached is:

We should re-use SET_STORIES w/ a {v:2} to indicate it is a new format.
We should also add {v:1} to all other core event data structures.
We should throw or at least warn if we receive an event with a higher version number.

Note that 3. is very possible if someone is running a SB6.0 storybook with a ref to a SB7+ iframe. We should put this in place now! Will open a separate issue for this.

WDYT @ndelangen?

ndelangen · 2020-04-24T11:43:20Z

I think the v number should be storybook's major version?

I like the solution @tmeasday

tmeasday · 2020-04-28T10:19:38Z

I created an issue for the version thing: #10576

let's discuss the numbering there, I will just do v:2 here for now.

shilman · 2020-04-29T03:49:34Z

Some before & after perf numbers from official-storybook page load:

Before: Denormalized

preview (generated-entry): 1.4s
- pushTomanager: 1s
manager: 1.1s
- telejson parse: 755ms
- setStories: 355ms

After: Normalized

preview (generated-entry): 470ms
- pushTomanager: 200ms
manager: 1s
- telejson parse: 150ms
- setStories: 900ms

shilman

I made a few fixes @tmeasday

shilman · 2020-04-29T03:56:59Z

lib/api/src/index.tsx

@@ -350,7 +369,7 @@ export function useSharedState<S>(stateId: string, defaultState?: S) {
      [`${SHARED_STATE_SET}-client-${stateId}`]: (s: S) => setState(s),
    };
    const stateInitializationHandlers = {
-      [SET_STORIES]: () => {
+      [SET_STORY_STORE_DATA]: () => {


I thought we were using SET_STORIES instead of SET_STORY_STORE_DATA? Looks like it only got partially reverted?

tmeasday · 2020-04-29T04:57:27Z

Thanks for taking those timings @shilman. We could definitely consider not denormalizing/combining parameters on stories in the manager until they are rendered although that could have a bunch of implications that we might not want.

We could also memoize the (globalParameters, kindParameters[kind]) part of the combination pretty easily..?

shilman · 2020-04-29T06:31:20Z

@tmeasday we can probably optimize further in a separate PR. before i merge this one, can you look at the chromatic changes? one appears to be related to the timing change and is probably OK (tho weird?!), but the other looks like it's possibly a bug, where argTypes is not getting passed through as a parameter. not obvious to me what's going on there.

tmeasday · 2020-04-29T07:56:41Z

lib/client-api/src/story_store.ts

@@ -333,7 +322,7 @@ export default class StoryStore {
        ...accumlatedParameters,
        argTypes: enhancer({
          ...identification,
-          storyFn,
+          storyFn: original,


@shilman I think the original version of this was looking at the wrong storyFn?

No, the enhancer needs the original story function. Maybe we should be passing that as original instead of storyFn??

I think this "fix" actually introduces a bug. Because the enhancer can't look at whether the original story function accepts args.

OK, I misunderstood. You fixed the bug by reverting that part of the PR.

Ok so I just fixed a bug in my own code 👍

Store parameters in the store in a normalized way

9ae73c7

tmeasday added the in progress label Apr 10, 2020

tmeasday commented Apr 10, 2020

View reviewed changes

lib/client-api/src/story_store.ts Show resolved Hide resolved

tmeasday added 8 commits April 21, 2020 21:19

Merge remote-tracking branch 'origin/next' into 10361-normalize-param…

e92b44e

…eters

Fix bad merge

7307b76

Fix story sort tests

7adf8c0

Combine parameters in fromId()

4a87d5a

We'll always define parameters.argTypes

198a601

Is this bad @shilman?

Normalize parameters in the SET_STORIES event

603dfc5

Rename SET_STORIES to SET_STORY_STORE_DATA

bd9dd9f

Pick up in the manager

9b91f38

tmeasday force-pushed the 10361-normalize-parameters branch from a415dc3 to 9b91f38 Compare April 22, 2020 13:21

Duplicate types and combineParameters

01867f9

Because circular deps

tmeasday requested a review from ndelangen April 22, 2020 13:41

tmeasday assigned shilman and ndelangen Apr 22, 2020

Bugfix

1bdd48b

tmeasday commented Apr 22, 2020

View reviewed changes

shilman reviewed Apr 22, 2020

View reviewed changes

lib/client-api/src/story_store.ts Outdated Show resolved Hide resolved

lib/client-api/src/story_store.ts Show resolved Hide resolved

lib/client-api/src/story_store.ts Show resolved Hide resolved

ndelangen added 2 commits April 23, 2020 10:42

Merge branch 'next' into 10361-normalize-parameters

febec0f

# Conflicts: # lib/api/src/modules/stories.ts

ADD rest data to ref

cb28194

tmeasday removed the in progress label Apr 28, 2020

tmeasday added 2 commits April 28, 2020 21:13

Explicitly don't allow setting storySort any other way

63682a2

Export combineParameters and document in migrations

c0cdd3f

tmeasday requested review from alterx and igor-dv as code owners April 28, 2020 11:39

Merge branch 'next' into 10361-normalize-parameters

afdfd04

shilman added 3 commits April 29, 2020 11:58

Fix typo

12a2853

SET_STORY_STORE_DATA => SET_STORIES

884bb15

Normalized parameters: Fix failing test

27fadc6

shilman approved these changes Apr 29, 2020

View reviewed changes

shilman changed the title ~~Store parameters in the store in a normalized way~~ Core: Store normalized parameters in the store Apr 29, 2020

shilman changed the title ~~Core: Store normalized parameters in the store~~ Core: Store normalized parameters Apr 29, 2020

shilman changed the title ~~Core: Store normalized parameters~~ Core: Make parameters normalized in store Apr 29, 2020

shilman changed the title ~~Core: Make parameters normalized in store~~ Core: Make parameters normalized in store/channel Apr 29, 2020

shilman changed the title ~~Core: Make parameters normalized in store/channel~~ Core: Normalize parameters in store/channel Apr 29, 2020

shilman added BREAKING CHANGE core labels Apr 29, 2020

shilman added 2 commits April 29, 2020 12:17

Fix inclusion logic

8390c62

Fix deepscan

03425f6

shilman and others added 2 commits April 29, 2020 15:20

Merge branch 'next' into 10361-normalize-parameters

c0f8fb2

Ensure we set parameters.argTypes in the preview

241d06c

tmeasday commented Apr 29, 2020

View reviewed changes

shilman added 2 commits April 29, 2020 16:16

Fix failing tests

a1b9372

Update snapshots

e2d54f6

tmeasday requested a review from alexandrebodin as a code owner April 29, 2020 08:33

shilman merged commit 94c6e1d into next Apr 29, 2020

shilman mentioned this pull request Apr 29, 2020

Normalize parameter storage #10361

Closed

tmeasday deleted the 10361-normalize-parameters branch April 29, 2020 10:08

This was referenced May 20, 2020

Core: Zero-config TypeScript loading #10813

Merged

Addon-docs: Extract ArgType parameters by component #10844

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Normalize parameters in store/channel #10373

Core: Normalize parameters in store/channel #10373

tmeasday commented Apr 10, 2020 •

edited

Loading

github-actions bot commented Apr 10, 2020 •

edited

Loading

tmeasday commented Apr 22, 2020

tmeasday Apr 22, 2020 •

edited

Loading

tmeasday Apr 22, 2020

shilman left a comment

ndelangen commented Apr 23, 2020

tmeasday commented Apr 24, 2020

ndelangen commented Apr 24, 2020

tmeasday commented Apr 28, 2020 •

edited

Loading

shilman commented Apr 29, 2020 •

edited

Loading

shilman left a comment

shilman Apr 29, 2020

tmeasday commented Apr 29, 2020 •

edited

Loading

shilman commented Apr 29, 2020

tmeasday Apr 29, 2020

shilman Apr 29, 2020

shilman Apr 29, 2020

tmeasday Apr 29, 2020

Core: Normalize parameters in store/channel #10373

Core: Normalize parameters in store/channel #10373

Conversation

tmeasday commented Apr 10, 2020 • edited Loading

github-actions bot commented Apr 10, 2020 • edited Loading

tmeasday commented Apr 22, 2020

tmeasday Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

tmeasday Apr 22, 2020

Choose a reason for hiding this comment

shilman left a comment

Choose a reason for hiding this comment

ndelangen commented Apr 23, 2020

tmeasday commented Apr 24, 2020

ndelangen commented Apr 24, 2020

tmeasday commented Apr 28, 2020 • edited Loading

shilman commented Apr 29, 2020 • edited Loading

Before: Denormalized

After: Normalized

shilman left a comment

Choose a reason for hiding this comment

shilman Apr 29, 2020

Choose a reason for hiding this comment

tmeasday commented Apr 29, 2020 • edited Loading

shilman commented Apr 29, 2020

tmeasday Apr 29, 2020

Choose a reason for hiding this comment

shilman Apr 29, 2020

Choose a reason for hiding this comment

shilman Apr 29, 2020

Choose a reason for hiding this comment

tmeasday Apr 29, 2020

Choose a reason for hiding this comment

tmeasday commented Apr 10, 2020 •

edited

Loading

github-actions bot commented Apr 10, 2020 •

edited

Loading

tmeasday Apr 22, 2020 •

edited

Loading

tmeasday commented Apr 28, 2020 •

edited

Loading

shilman commented Apr 29, 2020 •

edited

Loading

tmeasday commented Apr 29, 2020 •

edited

Loading