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() returns nothing for a docs page #21798

Closed
quantizor opened this issue Mar 28, 2023 · 41 comments · Fixed by #22118 or #22154
Closed

[Bug]: useParameter() returns nothing for a docs page #21798

quantizor opened this issue Mar 28, 2023 · 41 comments · Fixed by #22118 or #22154
Assignees

Comments

@quantizor
Copy link

quantizor commented Mar 28, 2023

Describe the bug

I'm attempting to upgrade a storybook 6 project to storybook 7 and am running into an issue where the storybook-dark-mode doesn't work for docs pages.

I've narrowed down the issue to @storybook/api useParameter('darkMode') returning an empty object inside the addon, but only for rendering docs pages.

To Reproduce

I don't have a reproduction at the moment, but a basic storybook v7 project with storybook-dark-mode added should exhibit the behavior. In chrome devtools I looked up the manager.js file from storybook-dark-mode and added a breakpoint where it uses the useParameter hook.

My guess is the parameters store isn't getting initialized for docs pages for some reason?

System

No response

Additional context

No response

@shilman
Copy link
Member

shilman commented Mar 29, 2023

This is an intentional breaking change. I couldn't find it in MIGRATION.md, but maybe I just missed it. I realize that doesn't solve your problem though.

@tmeasday can you please weigh in on this? storybook-dark-mode is a popular addon, and although we don't maintain it, it would be great if we could figure out how to get it updated.

cc @jonniebigodes @integrayshaun

@tmeasday
Copy link
Member

tmeasday commented Apr 3, 2023

Hey @probablyup, thanks for calling this out. We've taken another look at this.

The root of the problem is that with the new docs architecture, when you view docs there isn't an underlying story. In 6.5 there always was (although it was a bit of a kludge in the case of .mdx files). What that meant in 6.5 was that you could always get parameters when visiting a docs page, because there'd always be a story to read them off. In the case of .mdx files, that was just the project-level parameters (preview.js), which happened to work great for this addon.

In any case, we did look at this problem when migrated some of our own addons but ultimately decided in the cases where they needed useParameter() it didn't really make sense to even do that on docs pages, and so we thought the changed behaviour was probably the best way forward.

Having seen this example, it does seem like there are/will be other addons that do need the project-level parameters to be available all the time. So we'll look at making it work again on docs pages.

As we are super late in the 7.0 release cycle, we need to be a bit careful about how we do this. We'll likely release the new mechanism in a 7.1 alpha in the near future, test it a little, and backport it as a bug fix in 7.0.x patch release.

@raphaelokon
Copy link

@tmeasday Cheers for calling this one out. We also noticed the missing parameters when calling renderLabel in the sidebar for our add-on. We only ever get the parameters for the current active story via storybookAPI.getParameters(item.id) … any sibling story is getting null. Is this related @shilman ?

@shilman
Copy link
Member

shilman commented Apr 13, 2023

Sibling story within the same component? @tmeasday Would know best here

@raphaelokon
Copy link

@shilman Aye. Interestingly once I browse the other sibling story it gets the parameters and the label is displayed for both.

@tmeasday
Copy link
Member

That's a limitation of the "on demand" nature of the store. We can't know the parameters of a story until it has been rendered

@quantizor
Copy link
Author

Can’t you use what’s provided in preview.js as a global default?

@tmeasday
Copy link
Member

tmeasday commented Apr 14, 2023

@probablyup yes, in fact as of #22071 we'd supply the component's parameters (ie. what's defined in the default export of the CSF file + the project's parameters defined in preview.js) in this case.

Would that be a good or a bad thing for your use case @raphaelokon? So your call of storybookAPI.getParameters(item.id) would return:

  • the project's parameters before you render any story for the component
  • the component's parameters before you render story in question (if you render a sibling)
  • the story's parameters once you've rendered the story.

The inconsistency doesn't sit super well with me but maybe it's better than nothing as @probablyup implies.

As an aside the issue of not getting a sibling story's parameters is an issue in the v7 (on demand) store prior to 7.0, IIUC, whereas this bug specifically talks about a regression in docs pages in 7.0 (just to be clear for anyone following along).

@raphaelokon
Copy link

raphaelokon commented Apr 14, 2023

@probablyup I’ll check it out. It was just working before inside renderLabel because we could call getParameters to fetch them even for story ids of siblings. Can you elaborate a bit on what you mean by using the preview.js? @probablyup You mean importing them like import { globalTypes, parameters } from './preview'?

@tmeasday Yeah, that would totally work for me, cheers.

As an aside the issue of not getting a sibling story's parameters is an issue in the v7 (on demand) store prior to 7.0, IIUC, whereas this bug specifically talks about a regression in docs pages in 7.0 (just to be clear for anyone following along).

@tmeasday Sorry, I do not wanted to spam your docs issue, merely just mention it as it may be related and we kept seeking for a solution :-)

@tmeasday
Copy link
Member

tmeasday commented Apr 14, 2023

@raphaelokon no problem at all, in fact it was helpful to think about this use case! I was just noting it because the focus here is to fix the regression in 7.0, rather than necessarily solve every problem with useParameter()/api.getParameters().

@tmeasday
Copy link
Member

tmeasday commented Apr 18, 2023

@raphaelokon so after some discussion with @shilman, we are going to take a simpler approach in #22118.

What this will mean is that getParameters() will still display the current behaviour for stories that haven't been rendered yet (that is returning nothing).

There seem to be two clear use cases for knowing the project/component parameters on the manager side:

  1. To fallback if we don't yet know the story's parameters, if you are examining a story that's not the current story (your use case).
  2. To fallback as we wait for the story to prepare ([Bug]: useParameter behaviour as you change between stories #22119). In this case it isn't clear that falling back is even the correct solution, would love to hear people's thoughts.

So regarding 1. I'd like to hear more about your use case. Which parameters do you need exactly to renderLabel? Do they change between components/stories?

@quantizor
Copy link
Author

I’d really like to avoid returning nothing during render to avoid FOUC inside the preview window. It would make the dark mode addon have imperfect UX since it couldn’t proactively set dark mode based on config in preview.js.

Really would just love parameters exported from preview to be the default, it would solve all problems I’m aware of.

@tmeasday
Copy link
Member

tmeasday commented Apr 19, 2023

We actually have the perfect mechanism for this, tags. It's not a properly released feature yet, but we already use it for autodocs: https://storybook.js.org/docs/react/writing-docs/autodocs#setup-automated-documentation

Tags are different from parameters as they are static and part of the index, so they are available immediately as soon as the menu is rendered. I think the tags are available on the item passed to renderLabel() already!

@raphaelokon
Copy link

Oh wow! @tmeasday cheers for the link! It seems like this is exactly what we want.

@raphaelokon
Copy link

@tmeasday We tested it using tags: ['beta', ...], this works good, but ultimately we need to filter the relevant tags out of the collection of tags and assign meaning to it. Nothing too bad, but having a dedicated parameter (even if it is static) that can be declaratively queried is just sooo much better. We will be using tags for now but had to rewrite our addon a bit.

@tmeasday
Copy link
Member

tmeasday commented Apr 20, 2023

@raphaelokon would it make sense to use a field in main.js to configure that? Something like:

addons: [{ name: 'your addon', options: { x: 'y' } }]

I don't believe it's currently simply to get those options in the manager, but we could make it so.


Or do you even use the configuration in the preview? If not it'd make sense to configure your addon in .storybook/manager.js

@raphaelokon
Copy link

We do not use the add-on in the preview—solely to display/enhance the sidebar view items (story names) in Storybook with beta or deprecated labels. And we configure our add-on in the manager.js currently. As I said tags work for us, but it would also be great to use useParameter in stories to get more complex data for stories.

Cheers for all your help @tmeasday

@tmeasday
Copy link
Member

Oh, what kind of per story data would you use?

@raphaelokon
Copy link

In our case it was just an object literal containing meta information for the status →

// MyComponent.stories.js
export default {
  title: 'Components/MyComponent',
  component: MyComponent,
  parameters: {
    status: {
      type: 'deprecated',
      since: '2.0.0',
      altText: 'Additional accessible information about what the `deprecated` tag means',
      // → This could contain even more related data to this specific status.
    },
  },
}

This data cannot be expressed through tags → string[].

@tmeasday
Copy link
Member

This could contain even more related data to this specific status

Would this specific information be relevant to the component/story in question?

@shilman
Copy link
Member

shilman commented Apr 24, 2023

¡Ay Caramba!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.1.0-alpha.8 containing PR #22154 that references this issue. Upgrade today to the @future NPM tag to try it out!

npx sb@next upgrade --tag future

@shilman
Copy link
Member

shilman commented Apr 24, 2023

Jeepers creepers!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.7 containing PR #22120 that references this issue. Upgrade today to the @latest NPM tag to try it out!

npx sb@latest upgrade

@quantizor
Copy link
Author

quantizor commented Apr 24, 2023

@tmeasday Tested this out and nothing appears to have changed for 7.0.7. Is the useParameter hook re-triggered when that DOCS_PREPARED event is emitted? Trying to understand how to patch https://github.com/hipstersmoothie/storybook-dark-mode/blob/master/src/Tool.tsx to support the new scenario.

@tmeasday
Copy link
Member

@probablyup I don't believe any changes should be necessary.

I tried it out on my repro repo with 7.0.7 and the v6 store, and it appears to work as I expected. What am I missing?

https://github.com/tmeasday/test-dark-mode/tree/dark-mode-7-ssv6

@quantizor
Copy link
Author

Sigh, I don’t know what the deal is then. I’ve been very on top of upgrading these libraries and honestly it’s been a misery.

@tmeasday
Copy link
Member

What (mis-) behaviour are you seeing?

@quantizor
Copy link
Author

quantizor commented Apr 26, 2023

I upgraded to 7.0.7 and ran yarn dedupe to ensure everything is on the proper version, verified the lockfile etc. dark mode addon still is showing empty for the useParameter hook on a docs page. I’m specifically providing darkClass and lightClass which should be present and currently are not…

@tmeasday
Copy link
Member

tmeasday commented Apr 26, 2023

Hmmm, that's pretty weird. I'm not sure what to suggest there, can you turn on logLevel: 'debug' and check the console messages for the setIndex event? It should have the parameters in there for the docs entries. In 7.0.6 it wouldn't have parameters for docs entries. In 7.0.7:

image

@quantizor
Copy link
Author

Will try that and report back thanks!

@tmeasday
Copy link
Member

tmeasday commented May 3, 2023

I think something's gone wrong with the deployment script @shilman as this PR is listed in the changelog both for 7.0.7 and 7.0.8: https://github.com/storybookjs/storybook/blob/main/CHANGELOG.md#707-april-24-2023

@shilman
Copy link
Member

shilman commented May 4, 2023

@tmeasday my bad. Will fix by hand and hopefully the new release workflow helps

@nareshbhatia
Copy link

Hard to follow how to get this fix. I am at v7.0.11 and still seeing the issue. Do I have to wait till 7.1.0 gets released?

@tmeasday
Copy link
Member

@nareshbhatia the fix should be in 7.0.7. What is the behaviour you are (not) seeing?

@nareshbhatia
Copy link

@tmeasday, I am using storybook-dark-mode@3.0.0 with storybook@7.0.11. When I turn on dark mode, only the storybook UI changes to dark mode, the story itself retains the white background. See below:

image

If I look at the markup in chrome debugger, the theme-dark class is correctly applied to the <html> element, but <body> class does not get a dark background:

image

@tmeasday
Copy link
Member

@nareshbhatia hmm, that sounds like a different issue than what this ticket is about [1].

I'm not sure about the behaviour you are seeing. If you can post a reproduction that shows it working 6.5 and broken in 7.0 I can take a look, but maybe in a new ticket?

[1] this ticket is about (a) docs entries and (b) the manager side, ie. the sidebar etc.

@nareshbhatia
Copy link

Sounds good, @tmeasday. I will create a reproduction and a new issue.

@shubhamdhingra007
Copy link

@nareshbhatia, did you find any resolution for this?

@nareshbhatia
Copy link

@shubhamdhingra007, it's still on my TODO list. Haven't gotten to it yet.

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