-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Addon-docs: Refactor DocsRender/Context #18635
Conversation
☁️ Nx Cloud ReportCI 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 targetSent with 💌 from NxCloud. |
cb70145
to
7d6a4d8
Compare
0a04719
to
abd9404
Compare
7d6a4d8
to
8c5b2cd
Compare
Simplifies things down a lot!
And make sure doc blocks use the "primary" story by id when possible.
2c446a2
to
8705ae1
Compare
// 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); | ||
}; | ||
} |
There was a problem hiding this comment.
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.
@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 reactOptions: {
legacyRootApi: true,
}, No longer gets Regarding the flakiness. The reason is that the new storybook/renderers/react/src/render.tsx Lines 42 to 44 in 849cfca
@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. |
@tmeasday I don't see why that config/setting would be missing. That code didn't change AFAIK, the code is here: storybook/lib/builder-webpack5/src/preview/iframe-webpack.config.ts Lines 206 to 218 in 5683419
|
So that might mean getting the value coming from preset changed somehow.. I'm investigating. |
I figured it out... opening PR soon |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one 🙌
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:.mdx
files that may have a primary CSF file (<Meta of={}>
) as well as reference others (<Story ... meta={}>
)*.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)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 thecontext.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 theAddContext
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