Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: useParameter behaviour as you change between stories #22119

Open
tmeasday opened this issue Apr 17, 2023 · 19 comments
Open

[Bug]: useParameter behaviour as you change between stories #22119

tmeasday opened this issue Apr 17, 2023 · 19 comments

Comments

@tmeasday
Copy link
Member

tmeasday commented Apr 17, 2023

Describe the bug

  • Currently the manager only knows the parameter of stories it has rendered before.
  • When you select a story it emits a STORY_PREPARED event which reaches the manager after some delay.
  • Many addons (such as backgrounds) cannot work, and must hide themselves if they don't see the right parameter.

The net effect of this is when you browse between stories, there is a "flash of hidden toolbars" as the addons hide themselves while they wait for the new story to prepare.

We might also consider the behaviour as the first story loads, as similarly we don't have parameter information.

To Reproduce

  • Refresh the browser
  • Browse around a sandbox
  • Observe the backgrounds addon carefully.

System

No response

Additional context

To fix this, we might consider:

Falling back to the component + project parameters (we could make them available ala this unmerged PR: #22071)

This generally speaking would work well, but in some examples it might have similar issues of things flashing on and off as it loads. For example when you move between two stories with non-standard settings for the add (perhaps hiding it), the addon would briefly appear.

Continuing to return the previous story's value until the new story is prepared.

This might be complex to implement, but could be the best solution.

@raphaelokon
Copy link

@tmeasday Is this related to this one somehow? → #21798 (comment)

@tmeasday
Copy link
Member Author

Hey @raphaelokon, yes! Sorry I didn't link them together yet. This is basically a follow-on issue from that discussion that we'll think about for 7.1. Because we are in post-release mode for 7.0 we are trying to make the change there as small as possible, I hope that makes sense!

@quantizor
Copy link

Following up from the other issue:

The dark mode addon uses parameters for configuration storage, and I imagine many other addons do as well. Specifically, there is a darkClass and lightClass that can be user-supplied to match the selector used for scoping of light/dark mode styles in userland.

If docs stories do not have access to the parameters provided in preview.js, then there will be an unavoidable white FOUC for a user in dark mode as the iframe initializes that is jarring and did not previously happen under Storybook v6 (at least with storyStoreV7 off, which anyone who requires dynamic stories is likely to have configured.)

@tmeasday
Copy link
Member Author

tmeasday commented Apr 18, 2023

If docs stories do not have access to the parameters provided in preview.js, then there will be an unavoidable white FOUC for a user in dark mode as the iframe initializes that is jarring and did not previously happen under Storybook v6 (at least with storyStoreV7 off, which anyone who requires dynamic stories is likely to have configured.)

I don't think this is correct. You'd still be waiting for the initial SET_STORIES (v6) event to get parameters, which conceivably would take even longer than the initial STORY_CONFIGURED/DOCS_CONFIGURED (v7) event. A FOUC on startup is unavoidable if the addon (in the manager) relies on (parameter) data supplied by the preview.

What's different in the on demand store is the behaviour when you browse between stories. In the v6 store we had all the data always, so we could immediately return the new parameters. In the v7 store parameters are loaded as the story is loaded, so we can't immediately return the new parameters while we wait for the new story to render.

We need to figure out what to return in between. Should it be some fallback parameters (the project/component's parameters), the previous story's parameters, or null to indicate loading?

If it isn't the fallback parameters, should we add some API to allow addons to get those and fallback themselves?

@quantizor
Copy link

Here’s our storybook: http://ds.auslr.io/?path=/story/welcome--page

If you have system dark mode forced on and visit it you can see the current behavior… there’s definitely not a FOUC currently.

@raphaelokon
Copy link

@tmeasday Thanks so much for the follow up! If I can somehow help to test let me know.

@raphaelokon
Copy link

raphaelokon commented Apr 18, 2023

If it isn't the fallback parameters, should we add some API to allow addons to get those and fallback themselves?

I think a some addons may have the need to get story meta data a priori (or before rendering) to make assumptions about a story (at least ours do :D). A declarative way to fetch thosethis meta data would be awesome. At the same time I understand that keeping the API surface small is also something to strive for in terms of maintainability.

@tmeasday
Copy link
Member Author

tmeasday commented Apr 19, 2023

@probablyup there is a FOUC, it's just that the addon uses local storage to avoid it after the first time you visit the page. (If I am understanding you correctly at least--unless you mean something about light vs dark mode?)

Screen Recording 2023-04-19 at 10 55 33 am

Regaring addon-dark-mode, I just tried 6.5.x and 7.1.0-alpha.6 and AFAICT the behaviour is unchanged now 🚀 (let me know if I missed something). Try these two branches:

NOTE: the 7.1.0-alpha.6 version doesn't work in SSv6 mode yet, see: #22154

@tmeasday
Copy link
Member Author

A declarative way to fetch thosethis meta data would be awesome. At the same time I understand that keeping the API surface small is also something to strive for in terms of maintainability.

Unfortunately (a design decision that was taken long ago), parameters are completely dynamic and there's no way to get them without loading the story. We don't want to load all stories up front in the preview (for perf reasons), so to make it possible to fetch parameters for arbitrary stories from the manager would require some APIs to tell the preview to fetch a given story.

I'm inclined to say that's a bit complicated and we'd want a pretty convincing use case to justify that.

It sounds like in your case at least you'd want the parameters for all stories, which would sort of run up against the perf issue even if we had this API.

Generally speaking, I think we are leaning towards trying to use tags, which are static and part of the index (as discussed here) to help with these use cases. Tags are intentionally simple and simply a list of strings, so might not hit every use case, but let's see how we go!

@quantizor
Copy link

quantizor commented Apr 19, 2023

Tags are intentionally simple and simply a list of strings, so might not hit every use case, but let's see how we go!

Unless you can provide a config object in tags, I don’t see how that solves the problem…

Would it be possible to just introduce some blessed API that does what is needed: basic k-v storage available before stories are rendered that can be bootstrapped from preview.js

@tmeasday
Copy link
Member Author

tmeasday commented Apr 19, 2023

@probablyup when you say

solves the problem…

Are you talking about the initial FOUC due to waiting on a message from the preview? I am assuming you are (and I wonder if we should take this discussion to another issue if so, see below):

Would it be possible to just introduce some blessed API that does what is needed: basic k-v storage available before stories are rendered that can be bootstrapped from preview.js

Well project parameters could be that @probablyup, right? We don't currently send them to the manager on their own (I did briefly in #22071), but if that's the blessed way to share project-level configuration between preview + manager, then we could add the PROJECT_PREPARED event back [1]. Do you have thoughts on this @shilman?

Although it wouldn't solve the FOUC problem.

Parameters (for the current story) are available before the story is rendered. Just not before the manager is rendered. That's where the FOUC is coming from.

Really if an addon doesn't want an FOUC I suspect it should be configured on the manager side, or in main.js. I'm not really an expert on this part of the addon API though, perhaps @ndelangen wants to comment?

[1] Keep in mind the main reason I closed the PR is I didn't want to introduce a new feature in a patch release, and to take the time to think it through.


The use case we were discussing (and really what this issue is about) is around configuring things per-story differently. For that, we have:

  • parameters, which only are available when the story is loaded, but can contain more or less anything.
  • tags, which are available for all stories, immediately, but are limited to a list of strings.

We could add another concept but so far it seems like tags hit a lot of the "per story configuration I need to know about all the time" pretty well.

@quantizor
Copy link

quantizor commented Apr 19, 2023

Ultimately what I’m looking to achieve is a way for an addon to have config that’s available immediately from anywhere in storybook, regardless of docs or component story type.

I thought preview.js was the right place to initialize that, but if you’re saying it isn’t then yeah we need some documentation explaining the happy path for truly global addon config that doesn’t wait for stories or docs to be loaded before being available.

@ndelangen
Copy link
Member

a way for an addon to have config

Would this be static config? Or does this config change over time?

@quantizor
Copy link

Would this be static config? Or does this config change over time?

for storybook-dark-mode the config doesn’t really change between pages, so I’d say it’s static. As soon as storybook initializes, the config is actually copied into localStorage to make things faster for subsequent visits

@ndelangen
Copy link
Member

You could experiment with using the env feature.

This is a setting for webpack define plugin:


but also vite:
const envsRaw = await presets.apply<Promise<Builder_EnvsRaw>>('env');
let { define } = config;
if (Object.keys(envsRaw).length) {
// Stringify env variables after getting `envPrefix` from the config
const envs = stringifyProcessEnvs(envsRaw, config.envPrefix);
define = {
...define,
...envs,
};
}

and the manager too:
define: {
'process.env': JSON.stringify(envs),
...stringifyProcessEnvs(envs),
global: 'window',
module: '{}',
},

So if the data is stringifable (which it likely is, considering you're also putting it in local storage), that might work.

in your main.ts file, add a env entry, that's a async function.

export default {
  // .. 
  env: async (config) => ({
    ...config,
    SOME_UNIQUE_KEY: JSON.stringify({ your: 'value'}),
  })
};

I haven't tested this, I think it might work.

If you config object is very large this could have negative performance effects.

@shilman
Copy link
Member

shilman commented Apr 20, 2023

@tmeasday in DM you proposed a solution where the user can also export parameters from .storybook/manager.js to make sure they are instantly available in the manager.

The behavior would then be:

  • manager.js provides parameter defaults which are available on load
  • those parameters get overwritten by the preview parameters when stories are loaded

This means that you'll get a FOUC if you don't specify manager.js parameters OR if they differ from the story parameters. This feels like a really reasonable solution to me, though it does add some weird edge cases, e.g. will there be manager-only parameters if the user adds manager.js parameters that they don't add to the preview? do we still need localStorage? etc.

what does everybody else think?

@tmeasday
Copy link
Member Author

tmeasday commented Apr 21, 2023

So to be clear the solution @shilman is talking about is to define the them in a file that is imported both in manager.js and preview.js:

// .storybook/preview.js
import { themes } from './themes.js';

export default {
  parameters: {
    darkMode: themes
  }
};

// .storybook/manager.js
import { themes } from './themes.js';
import { configure } from '<addon>';

configure(themes);

The main downside I can see with this is it's a little harder for the user to configure. However it's not that hard and probably adding a new concept to our API to deal with this problem is hard to swallow.

I think the main alternative suggestion is something automated that behind the scenes works like @ndelangen is suggesting, basically that you configure the addon in main.js and SB wires those options through to the manager+preview somehow. So something like:

// user .storybook/main.js
export default {
  addons: [{ name: 'storybook-dark-mode', options: { dark: {...}, light: {...} } }],
}

// addon code, perhaps this API would work in both manager+preview contexts:
const { dark, light } = getAddonOptions('storybook-dark-mode');

@ndelangen
Copy link
Member

@tmeasday that sounds like a great start for a API proposal tbh, should we create an RFC for that?

@tmeasday
Copy link
Member Author

I guess so @ndelangen. I'm still unsure if it's useful enough to commit to it, but perhaps an RFC will help us tease that out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants