Core: Add story state to store #9817

tmeasday · 2020-02-11T10:34:50Z

For #9811. Based on #9810 (need to change base when this is merged).

This is just the basics in the story store (doesn't do anything on the manager side, and no hooks exist yet). Opening PR now in case there is any discussion. Some notes, questions:

Previously the story store has not listened to events itself (even though it has access to the channel), and all communication w/ the manager has been mediated by preview/start.js. I kind of feel like that complicates things unnecessarily in this case (when the manager is sending a message to change the state) and probably other existing cases too, TBH.
I don't fully understand why the story store both is an event emitter and also emits events on the channel. Is this a historical thing due to React Native (where the channel can not exist)? Do we still need it? It seems confusing and it might be the time to get rid of it.
Should I use the force re-render mechanism to re-render the story when the state changes, or should there be explicit state checks in the renderMain function? It kind of feels like forceRender is a reasonable fit for what we are doing here.
Should we merge this PR (which AFAICT doesn't break anything) and iterate or would people prefer me to keep going on this branch?

tmeasday · 2020-02-11T10:35:12Z

lib/client-api/src/story_store.test.ts

@@ -258,29 +319,4 @@ describe('preview.story_store', () => {
      expect(store.fromId(toId('kind-1', 'story-1.2'))).toBeTruthy();
    });
  });
-
-  describe('story sorting', () => {


There is a whole other, larger sorting test.

lib/client-api/src/story_store.ts

github-actions · 2020-02-11T10:37:04Z

	Fails
🚫	PR is not labeled with one of: ["cleanup","doc-dependencies:update","BREAKING CHANGE","feature request","bug","documentation","maintenance","dependencies:update","dependencies","other"]

Generated by 🚫 dangerJS against 654a508

atanasster · 2020-02-11T10:55:47Z

Definitely not refactor of useStoryState - and good riddance :)
Some of things you have right, some you will find out once you travel that road. most everything in my original code was done for a reason - like the channel listeners attached to the store, story re-render. I am also testing to unhook the channel listeners on channel.set
Haven't looked in depth but some notes

The code on the manager will be mostly the same and i would suggest to create a shared package to manage the state (and other shared functionality)
There are some subtle differences between manager and preview - any ‘functions’ will reside on the preview - like onClick and will not be actually serialized to the manager,
The notation ({ state : { name } }) is really ugly in my opinion, esp. if you want to reuse it for controls
Compare to ({ name }) - a beauty :)

The best approach in my opinion is to merge the changes in small batches - and i guess you can do that

atanasster · 2020-02-11T11:43:40Z

Further, in support of my comment above that ({ state : { name } }) is not the best design choice.

Here is a small video with the final result of my 'smart-controls' implementation - please note that just by adding props as a parameter, the controls are created and used automatically (counting just a minimal few keystrokes). This is pure perfection :)

atanasster · 2020-02-11T11:55:04Z

This is the entirety of the story code with my architecture:

export const controls = props => <Avatar {...props} />;

and state would go into the second story parameter, with the story context:

export const controls = (props, { state: { name }}) => <Avatar {...props} />;

tmeasday · 2020-02-11T22:51:32Z

I am also testing to unhook the channel listeners on channel.set

Yeah I am pretty sure we never change the channel on the story store so I am inclined not to add code that is never used. This is something else I want to confirm with @ndelangen. Probably we should make it so that it throws if channel is already set.

i would suggest to create a shared package to manage the state

Yeah, I'm puzzling over this.

There are some subtle differences between manager and preview - any ‘functions’ will reside on the preview - like onClick and will not be actually serialized to the manager,

Yeah, state is not going to include functions.

The notation ({ state : { name } }) is really ugly in my opinion, esp. if you want to reuse it for controls
Compare to ({ name }) - a beauty :)

I know, but as discussed in the original design document we will iterate our way there, probably changing to that as the default in 7.0 if it works in the wild. In the meantime we'll add an option that makes the state the first argument to the story function.

atanasster · 2020-02-11T23:13:41Z

Yeah, state is not going to include functions.

So is state going to replace my controls implementation or are the two going to co-exist
Tom can you please explain in details what you intend to accomplish as far as architecture goes.

I know, but as discussed in the original design document we will iterate our way there, probably changing to that as the default in 7.0 if it works in the wild. In the meantime we'll add an option that makes the state the first argument to the story function.

Hmm - my code is already handling both legacy and "new" way to pass the properties/controls. It works great.

In the meantime, I am finishing a version of component controls that works with storybook 5.3 - its a botched down version (lets call it the useStoryState version :)) - and that makes me even more enthusiastic with what I accomplished with my full implementation. Lets try to get to that level with 6.0

shilman

Looks good to me. We'd discussed some idiomatic ways to use this, and it would probably be useful to have those in a second PR before merging this.

shilman · 2020-02-12T01:39:32Z

lib/core/src/client/preview/start.js

@@ -350,6 +350,7 @@ export default function start(render, { decorateStory } = {}) {
  });

  storyStore.on(Events.STORY_RENDER, renderUI);
+  storyStore.on(Events.STORY_STATE_CHANGED, () => renderUI(true));


Suggested change

storyStore.on(Events.STORY_STATE_CHANGED, () => renderUI(true));

storyStore.on(Events.STORY_STATE_CHANGED, forceReRender);

shilman · 2020-02-12T01:40:15Z

lib/client-api/src/story_store.ts

+  setStoryState(id: string, newState: StoryState) {
+    if (!this._data[id]) throw new Error(`No story for id ${id}`);
+    const { state } = this._data[id];
+    this._data[id].state = { ...state, ...newState };


Is there any way to delete something from state? Do we care?

Not currently. Let's wait until that use case comes up, maybe?

tmeasday · 2020-02-12T02:43:37Z

I am finishing a version of component controls that works with storybook 5.3

That's great, can't wait to see it!

I'll update this PR with some more details documenting explaining how state will work in 6.0, but the short story is that we want to do this thing incrementally so we can feel out the feature as we go without committing to a bunch of new APIs before the community have had a chance to use them.

atanasster · 2020-02-12T03:02:57Z

I am finishing a version of component controls that works with storybook 5.3

That's great, can't wait to see it!

It's a bit of a disappointment really. I mean its better than knobs etc., but I honestly have seen perfection with my full architecture in place and can't stop thinking about it. Did you actually run my branch? Everything was fully functioning with batteries included.

Anyway, regardless of anyone's reluctance I will find a way to make it work the right way.

I'll update this PR with some more details documenting explaining how state will work in 6.0, but the short story is that we want to do this thing incrementally so we can feel out the feature as we go without committing to a bunch of new APIs before the community have had a chance to use them.

Yeah, no offense meant but thats a bit of a mistake - focus on implementation details and later fit in the end goal ( I will not mention useStoryState because that one even the implementation details were crooked)

As you remember, I wrote first the selection of end goals, and the best design was. Only then did I go into making it work (I did choose the hardest option to make, just because it was so much better architecture).

export const controls = props => <Avatar {...props} />;

This specific design was discussed with @shillman, including it was his suggestion was added the option to use legacy calling convention.

tmeasday · 2020-02-12T03:12:42Z

Yeah, no offense meant but thats a bit of a mistake - focus on implementation details and later fit in the end goal

🤷‍♂ we found this approach worked really well for SB Docs, TBH.

( I will not mention useStoryState because that one even the implementation details were crooked)

Can we stop doing that? Regardless of how close the current implementation of useStoryState is to being complete, I don't think it is in the spirit of this community to keep talking negatively about other's code (I realise you were involved in the code too, but still).

This specific design was discussed with @shillman, including it was his suggestion was added the option to use legacy calling convention.

Yep, I still plan to add this feature in this or a follow up PR.

atanasster · 2020-02-12T03:25:12Z

Can we stop doing that? Regardless of how close the current implementation of useStoryState is to being complete, I don't think it is in the spirit of this community to keep talking negatively about other's code (I realise you were involved in the code too, but still).

Actually I am all for it - I dont really care about code, or naming variables - just "what do you want to accomplish". Sooner or later all code becomes obsolete but ideas persist.

tmeasday · 2020-02-12T04:38:14Z

Actually I am all for it - I dont really care about code, or naming variables - just "what do you want to accomplish".

Well others do, and the code was written by real people in a community who in most cases are volunteering their time. It is important to set a tone that criticising others in this way isn't acceptable in the community.

tmeasday · 2020-02-12T06:42:56Z

@atanasster I think you edited my comment by mistake, let me put what you said in quotes here:

Not sure if we are disagreeing on this ?
Just in case - 100% agreed, and I have not criticized any code - useStoryState was just an idea and I did try my best to make it work but it simply wasn't the right idea or the right implementation. The current code is not related to useStoryState in any form or shape.

Oh, maybe I misunderstood what you meant above when you said "Actually I am all for it", apologies.

I realise the history on useStoryState, I just don't want us to be publicly disparaging code or features here on GH or elsewhere because others reading it don't have all the context and can get the wrong impression about how the community operates. As prominent members of the community we set the tone.

I am also a member of the community, and I did spend a lot of time volunteering. I really hope my efforts are also appreciated - and I did spend an outsized amount of time on this feature.

Of course they are!

Anyway, I will leave you at this - if you need my input please let me know.

👍 thanks.

stale · 2020-03-06T23:28:11Z

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!

stale · 2020-04-06T00:22:37Z

Hey there, it's me again! I am going close this issue to help our maintainers focus on the current development roadmap instead. If the issue mentioned is still a concern, please open a new ticket and mention this old one. Cheers and thanks for using Storybook!

tmeasday added 8 commits February 11, 2020 17:27

Refactor story_store.test slightly to get rid of linting errors

523be64

Fix linting and remove redundant test

7344654

Remove redundant stuff from story_store

ca79c64

Add very basic story state

45e5c5b

Add STORY_STATE_CHANGED event

a6ffc8f

Listen to CHANGE_STORY_STATE and change it in the store

654a562

Ensure that the state is passed to the story function

fdf34c5

Ensure the story re-renders when the state changes

654a508

tmeasday requested review from alterx, igor-dv and usulpro as code owners February 11, 2020 10:34

tmeasday commented Feb 11, 2020

View reviewed changes

shilman changed the title ~~Add story state to store~~ Core: Add story state to store Feb 12, 2020

shilman added core feature request labels Feb 12, 2020

shilman added this to the 6.0.0 milestone Feb 12, 2020

shilman approved these changes Feb 12, 2020

View reviewed changes

tmeasday changed the base branch from 9507-remove-legacy-data to next February 14, 2020 22:33

stale bot added the inactive label Mar 6, 2020

stale bot closed this Apr 6, 2020

dependencies bot mentioned this pull request Jan 15, 2021

Update 28 dependencies from npm #2806

Merged

stof deleted the 9811-add-story-state branch May 25, 2022 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Add story state to store #9817

Core: Add story state to store #9817

tmeasday commented Feb 11, 2020

tmeasday Feb 11, 2020

github-actions bot commented Feb 11, 2020

atanasster commented Feb 11, 2020

atanasster commented Feb 11, 2020

atanasster commented Feb 11, 2020

tmeasday commented Feb 11, 2020 •

edited

Loading

atanasster commented Feb 11, 2020 •

edited

Loading

shilman left a comment

shilman Feb 12, 2020

shilman Feb 12, 2020

tmeasday Feb 12, 2020

tmeasday commented Feb 12, 2020

atanasster commented Feb 12, 2020

tmeasday commented Feb 12, 2020 •

edited

Loading

atanasster commented Feb 12, 2020

tmeasday commented Feb 12, 2020 •

edited

Loading

tmeasday commented Feb 12, 2020 •

edited

Loading

stale bot commented Mar 6, 2020

stale bot commented Apr 6, 2020

	storyStore.on(Events.STORY_STATE_CHANGED, () => renderUI(true));
	storyStore.on(Events.STORY_STATE_CHANGED, forceReRender);

Core: Add story state to store #9817

Core: Add story state to store #9817

Conversation

tmeasday commented Feb 11, 2020

tmeasday Feb 11, 2020

Choose a reason for hiding this comment

github-actions bot commented Feb 11, 2020

atanasster commented Feb 11, 2020

atanasster commented Feb 11, 2020

atanasster commented Feb 11, 2020

tmeasday commented Feb 11, 2020 • edited Loading

atanasster commented Feb 11, 2020 • edited Loading

shilman left a comment

Choose a reason for hiding this comment

shilman Feb 12, 2020

Choose a reason for hiding this comment

shilman Feb 12, 2020

Choose a reason for hiding this comment

tmeasday Feb 12, 2020

Choose a reason for hiding this comment

tmeasday commented Feb 12, 2020

atanasster commented Feb 12, 2020

tmeasday commented Feb 12, 2020 • edited Loading

atanasster commented Feb 12, 2020

tmeasday commented Feb 12, 2020 • edited Loading

tmeasday commented Feb 12, 2020 • edited Loading

stale bot commented Mar 6, 2020

stale bot commented Apr 6, 2020

tmeasday commented Feb 11, 2020 •

edited

Loading

atanasster commented Feb 11, 2020 •

edited

Loading

tmeasday commented Feb 12, 2020 •

edited

Loading

tmeasday commented Feb 12, 2020 •

edited

Loading

tmeasday commented Feb 12, 2020 •

edited

Loading