Core: Restore `useParameter` behaviour for Docs entries #22071

tmeasday · 2023-04-13T05:45:22Z

What I did

Added two new events PROJECT_PREPARED and COMPONENT_PREPARED, analogous to STORY_PREPARED.
They transmit annotations (specifically argTypes and parameters) at appropriate times for components and the project.
Collect this information on the manager side:
- Component annotations get stored in the index (both for local + refs), similar to the story annotations
- Project annotations get stored at the top-level (both for local + ref)
Update getParameter (useParameter) to fall back to the component then the project if parameters are not available.

This means attached docs use the component's parameters -- in particular it means certain toolbars (backgrounds in essentials) appear again:

And unattached docs use the project's:

What I didn't do, but could in future:

Also store globals+globalTypes in the state.projectAnnotations and ref.projectAnnotations. This, plus an update to getGlobals[Types] would address Composite Storybook does not show toolbars for the other storybooks #17759.
A. Additional to this we should probably refactor the SET_GLOBALS event to just be part of PROJECT_PREPARED.
Send over (initial) args (or other annotations) at the component or project level (I'm not sure if we should?). Then again, are argTypes useful without initial args?

Refactor this code to read options from the project parameter rather than the first story, which seems like a good move!:

storybook/code/lib/manager-api/src/modules/stories.ts

Lines 435 to 442 in be000bb

    
           if (!ref) { 
        
             if (!store.getState().hasCalledSetOptions) { 
        
               const { options } = update.parameters; 
        
               fullAPI.setOptions(removeRemovedOptions(options)); 
        
               store.setState({ hasCalledSetOptions: true }); 
        
             } 
        
           }

How to test

Run a sandbox, check toolbar behaviour
Check the same for composition!
See tests

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"]

shilman

Amazing work. I have questions though...

shilman · 2023-04-13T12:28:31Z

code/lib/manager-api/src/modules/stories.ts

@@ -113,6 +119,10 @@ function removeRemovedOptions<T extends Record<string, any> = Record<string, any
  return result;
 }

+function toComponentId(storyId: StoryId): ComponentId {
+  return storyId.split('--')[0];


a little surprised we don't have a utility in @storybok/csf for this. no big deal.

Me too. I didn't want to mess around with new CSF packages at this moment in time, but can if you disagree.

Open a PR for this against the CSF package, but don't merge

shilman · 2023-04-13T12:31:49Z

code/lib/manager-api/src/modules/stories.ts

        }
      }

+      // If not prepared, or a unattached doc, get the project's parameters


what happens in the case it's not prepared? is there a race condition here? or is it handled somewhere else?

ok read more of the PR and it looks like COMPONENT_PREPARED is emitted in the story render or the CSF Docs render. Are there other cases that need to be accounted for?

shilman · 2023-04-13T12:37:10Z

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

@@ -745,6 +790,13 @@ describe('PreviewWeb', () => {
        expect(importFn).toHaveBeenCalledWith('./src/ComponentTwo.stories.js');
      });

+      it('DOES NOT emit COMPONENT_PREPARED', async () => {
+        document.location.search = '?id=introduction--docs&viewMode=docs';


So the component parameters wouldn't show in this case? Seems inconsistent?

There are no "component parameters" in this case as it is a MDX file.

We should emit the project parameters here.

shilman · 2023-04-13T12:39:17Z

code/lib/preview-api/src/modules/preview-web/render/MdxDocsRender.ts

@@ -29,6 +29,8 @@ import { DocsContext } from '../docs-context/DocsContext';
 export class MdxDocsRender<TRenderer extends Renderer> implements Render<TRenderer> {
  public readonly type: RenderType = 'docs';

+  public readonly subtype = 'mdx';


So I guess in this case, it would not emit a COMPONENT_PREPARED .. but does that mean that the store is in a perpetually unprepared state?

shilman · 2023-04-13T12:41:08Z

code/ui/manager/src/index.tsx

@@ -45,6 +45,7 @@ const Main: FC<{ provider: Provider }> = ({ provider }) => {
          docsOptions={global?.DOCS_OPTIONS || {}}
        >
          {({ state, api }: Combo) => {
+            console.log(useParameter('backgrounds'));


Suggested change

console.log(useParameter('backgrounds'));

JReinhold

Nice work! only a few minor requests

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

quantizor · 2023-04-14T14:19:17Z

@tmeasday thanks for working on this!

tmeasday · 2023-04-19T03:55:45Z

Closing in favour of #22118, a simpler solution.

Some of the advantages of this PR we can explore in 7.1 under #22119

tmeasday added 4 commits April 13, 2023 14:49

Remove unnecessary code

1239024

Refactor prepareMeta to cache on store

adaf048

Add PROJECT_PREPARED event

1e50704

Add new COMPONENT_PREPARED event

a91f63e

tmeasday added bug patch:yes Bugfix & documentation PR that need to be picked to main branch core labels Apr 13, 2023

tmeasday added 6 commits April 13, 2023 16:24

Store projectAnnotations in the manager

52a5d5e

Rename function

148d71a

Set component annotations on COMPONENT_PREPARED in the manager

bfa6c5d

Update getParameter to fall back to component+project

5c33ac4

Fix preparedMeta test

4d243ad

Add an E2E test of parameters behaviour

c66382f

tmeasday requested review from JReinhold, ndelangen and shilman April 13, 2023 11:51

tmeasday marked this pull request as ready for review April 13, 2023 11:54

shilman approved these changes Apr 13, 2023

View reviewed changes

tmeasday mentioned this pull request Apr 14, 2023

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

Closed

JReinhold reviewed Apr 14, 2023

View reviewed changes

code/lib/preview-api/src/modules/preview-web/PreviewWeb.test.ts Show resolved Hide resolved

code/lib/preview-api/src/modules/preview-web/PreviewWeb.test.ts Show resolved Hide resolved

code/lib/preview-api/src/modules/preview-web/PreviewWeb.test.ts Show resolved Hide resolved

This was referenced Apr 17, 2023

Core: Restore Docs useParameter using DOCS_PREPARED #22118

Merged

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

Open

valentinpalkovic assigned tmeasday and shilman and unassigned tmeasday Apr 17, 2023

tmeasday closed this Apr 19, 2023

stof deleted the tom/21798-fix-useParameter branch August 2, 2023 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Restore `useParameter` behaviour for Docs entries #22071

Core: Restore `useParameter` behaviour for Docs entries #22071

tmeasday commented Apr 13, 2023 •

edited by shilman

shilman left a comment

shilman Apr 13, 2023

tmeasday Apr 14, 2023 •

edited

tmeasday Apr 14, 2023

shilman Apr 13, 2023

shilman Apr 13, 2023

shilman Apr 13, 2023

tmeasday Apr 14, 2023

tmeasday Apr 14, 2023

shilman Apr 13, 2023

shilman Apr 13, 2023

JReinhold left a comment

quantizor commented Apr 14, 2023

tmeasday commented Apr 19, 2023

	if (!ref) {
	if (!store.getState().hasCalledSetOptions) {
	const { options } = update.parameters;
	fullAPI.setOptions(removeRemovedOptions(options));
	store.setState({ hasCalledSetOptions: true });
	}
	}

Core: Restore useParameter behaviour for Docs entries #22071

Core: Restore useParameter behaviour for Docs entries #22071

Conversation

tmeasday commented Apr 13, 2023 • edited by shilman

What I did

What I didn't do, but could in future:

How to test

Checklist

Maintainers

shilman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmeasday Apr 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JReinhold left a comment

Choose a reason for hiding this comment

quantizor commented Apr 14, 2023

tmeasday commented Apr 19, 2023

Core: Restore `useParameter` behaviour for Docs entries #22071

Core: Restore `useParameter` behaviour for Docs entries #22071

tmeasday commented Apr 13, 2023 •

edited by shilman

tmeasday Apr 14, 2023 •

edited