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

Addon-docs: Refactor DocsRender/Context #18635

Merged

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Jul 6, 2022

Telescoping on #18634

What I did

Refactored DocsContextProps to be a little bit simpler, and much more consistently implemented across the three different docs contexts:

  • Standalone - .mdx files that may have a primary CSF file (<Meta of={}>) as well as reference others (<Story ... meta={}>)
  • Template - *.stories.mdx files or docs page entries that are attached to one (or more, in the case of component stories spread across CSF files) CSF files. (NOTE this is the legacy case)
  • External - similar to Standalone, but running in an external system.

The APIs available to doc blocks are backwards compatible, with the following changes:

  • storyById can now be called with no parameters to get the primary story. This is preferred (and the blocks are changed) as the context.id may not be a story id in the standalone or external case.
  • mdxStoryNameToKey/mdxComponentAnnotations are no longer defined. There is a new API available, context.storyIdByName that serves the same purpose. It is used by our blocks to get ids for MDX defined stories. This is a breaking change, however I am not sure if it was ever used externally.

Now that the MDX compilers don't need to append mdxStoryNameToKey/mdxComponentAnnotations any more, there's no need for the AddContext utility either. I've got a task to do this cleanup later: https://linear.app/chromaui/issue/SB-465/remove-call-to-addtemplate-from-mdx-generators

@nx-cloud
Copy link

nx-cloud bot commented Jul 6, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit b50523d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@tmeasday tmeasday added maintenance User-facing maintenance tasks addon: docs labels Jul 6, 2022
@tmeasday tmeasday force-pushed the tom/sb-452-blocks-update-blocks-to-work-in-docspage branch from cb70145 to 7d6a4d8 Compare July 6, 2022 09:15
@ndelangen ndelangen marked this pull request as draft July 6, 2022 22:02
@tmeasday tmeasday force-pushed the no-title-prefix-for-meta-of branch from 0a04719 to abd9404 Compare July 7, 2022 02:26
@tmeasday tmeasday force-pushed the tom/sb-452-blocks-update-blocks-to-work-in-docspage branch from 7d6a4d8 to 8c5b2cd Compare July 7, 2022 02:27
@tmeasday tmeasday marked this pull request as ready for review July 7, 2022 02:28
Simplifies things down a lot!
And make sure doc blocks use the "primary" story by id when possible.
@tmeasday tmeasday force-pushed the tom/sb-452-blocks-update-blocks-to-work-in-docspage branch from 2c446a2 to 8705ae1 Compare July 7, 2022 04:55
Base automatically changed from no-title-prefix-for-meta-of to future/base July 9, 2022 05:44
Comment on lines -392 to -417
// Used by docs' modernInlineRender to render a story to a given element
// Note this short-circuits the `prepare()` phase of the StoryRender,
// main to be consistent with the previous behaviour. In the future,
// we will change it to go ahead and load the story, which will end up being
// "instant", although async.
renderStoryToElement(story: Story<TFramework>, element: HTMLElement) {
if (!this.renderToDOM)
throw new Error(`Cannot call renderStoryToElement before initialization`);

const render = new StoryRender<TFramework>(
this.channel,
this.storyStore,
this.renderToDOM,
this.inlineStoryCallbacks(story.id),
story.id,
'docs',
story
);
render.renderToElement(element);

this.storyRenders.push(render);

return async () => {
await this.teardownRender(render);
};
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was duplicated from the super class, I think from a merge conflict.

@tmeasday tmeasday requested a review from shilman July 11, 2022 06:00
@tmeasday
Copy link
Member Author

@ndelangen I got a flaky test run: https://app.circleci.com/pipelines/github/storybookjs/storybook/27512/workflows/ea52cb29-f334-4cb2-9192-f7c8df1c1bf6/jobs/394112

I tracked down the issue -- the problem is the project react_legacy_root_api, which sets (in main.js):

reactOptions: {
  legacyRootApi: true,
},

No longer gets FRAMEWORK_OPTIONS set in the preview, which means it renders in react 18 mode, which makes this test flaky. I'll discuss the flakiness below. But @ndelangen I assume the lack of globals in the preview is due to changes in setup that you might know about?


Regarding the flakiness. The reason is that the new react-dom/client API no longer has a "done" callback, so now we have a simple setTimeout(,0) before assuming the story is rendered:

setTimeout(() => {
resolve(null);
}, 0);

@valentinpalkovic I think we are going to need to come up with a better solution than that. The problem is that our "storybook hooks" that e.g. run inside the JSX decorator rely on the current story still being in the "render" phase at the time the hook is called to work properly.

@ndelangen
Copy link
Member

@tmeasday I don't see why that config/setting would be missing. That code didn't change AFAIK, the code is here:

globals: {
CONFIG_TYPE: configType,
LOGLEVEL: logLevel,
FRAMEWORK_OPTIONS: frameworkOptions,
CHANNEL_OPTIONS: coreOptions.channelOptions,
FEATURES: features,
PREVIEW_URL: previewUrl,
STORIES: stories.map((specifier) => ({
...specifier,
importPathMatcher: specifier.importPathMatcher.source,
})),
SERVER_CHANNEL_URL: serverChannelUrl,
},

@ndelangen
Copy link
Member

So that might mean getting the value coming from preset changed somehow.. I'm investigating.

@ndelangen
Copy link
Member

I figured it out... opening PR soon

@shilman shilman changed the title Refactor DocsRender/Context Addon-dcos: Refactor DocsRender/Context Jul 11, 2022
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one 🙌

@tmeasday tmeasday merged commit 8a45147 into future/base Jul 12, 2022
@tmeasday tmeasday deleted the tom/sb-452-blocks-update-blocks-to-work-in-docspage branch July 12, 2022 01:29
@shilman shilman changed the title Addon-dcos: Refactor DocsRender/Context Addon-docs: Refactor DocsRender/Context Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: docs maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants