-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Portable stories: Use internal Storybook renderers mechanism #28766
Changes from 2 commits
8f2c88d
afd11a8
d2965c9
b672f53
5631f17
2996b7f
64a9fa3
07babfe
2241c54
e30bfcc
6289999
257a945
072a951
76391dd
89f3a13
feccff5
e532994
3f96682
0760555
8fde44d
43c0733
8f39a19
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
export {}; | ||
|
||
declare const globalThis: { | ||
IS_REACT_ACT_ENVIRONMENT?: boolean; | ||
}; | ||
|
||
// TODO(9.0): We should actually wrap all those lines in `act`, but that might be a breaking change. | ||
// We should make that breaking change for SB 9.0 | ||
// Write issue | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider wrapping all lines in |
||
export function preventActChecks(callback: () => void): void { | ||
const originalActEnvironment = globalThis.IS_REACT_ACT_ENVIRONMENT; | ||
globalThis.IS_REACT_ACT_ENVIRONMENT = false; | ||
try { | ||
callback(); | ||
} finally { | ||
globalThis.IS_REACT_ACT_ENVIRONMENT = originalActEnvironment; | ||
} | ||
Comment on lines
+9
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Ensure callback does not rely on |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,14 @@ | ||
/* eslint-disable react/no-deprecated */ | ||
import type { ReactElement } from 'react'; | ||
import * as ReactDOM from 'react-dom'; | ||
import { preventActChecks } from './preventActChecks'; | ||
|
||
export const renderElement = async (node: ReactElement, el: Element) => { | ||
return new Promise<null>((resolve) => { | ||
ReactDOM.render(node, el, () => resolve(null)); | ||
preventActChecks(() => void ReactDOM.render(node, el, () => resolve(null))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Ensure |
||
}); | ||
}; | ||
|
||
export const unmountElement = (el: Element) => { | ||
ReactDOM.unmountComponentAtNode(el); | ||
preventActChecks(() => void ReactDOM.unmountComponentAtNode(el)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Verify |
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ import type { FC, ReactElement } from 'react'; | |
import * as React from 'react'; | ||
import type { Root as ReactRoot, RootOptions } from 'react-dom/client'; | ||
import * as ReactDOM from 'react-dom/client'; | ||
import { preventActChecks } from './preventActChecks'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Ensure |
||
|
||
// A map of all rendered React 18 nodes | ||
const nodes = new Map<Element, ReactRoot>(); | ||
|
@@ -26,15 +27,17 @@ export const renderElement = async (node: ReactElement, el: Element, rootOptions | |
const root = await getReactRoot(el, rootOptions); | ||
|
||
return new Promise((resolve) => { | ||
root.render(<WithCallback callback={() => resolve(null)}>{node}</WithCallback>); | ||
preventActChecks(() => | ||
root.render(<WithCallback callback={() => resolve(null)}>{node}</WithCallback>) | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Wrapping |
||
}); | ||
}; | ||
|
||
export const unmountElement = (el: Element, shouldUseNewRootApi?: boolean) => { | ||
const root = nodes.get(el); | ||
|
||
if (root) { | ||
root.unmount(); | ||
preventActChecks(() => root.unmount()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Confirm that |
||
nodes.delete(el); | ||
} | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ import { setProjectAnnotations, composeStories, composeStory } from '..'; | |
import type { Button } from './Button'; | ||
import * as stories from './Button.stories'; | ||
|
||
setProjectAnnotations([{ testingLibraryRender: render }]); | ||
setProjectAnnotations([]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Ensure that removing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not needed anymore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. setProjectAnnotations still register the react annoations. So I need to call it. And TS accepts only accepts an empty array, we probably should change that though? |
||
|
||
// example with composeStories, returns an object with all stories composed with args/decorators | ||
const { CSF3Primary, LoaderStory, MountInPlayFunction } = composeStories(stories); | ||
|
@@ -77,7 +77,6 @@ describe('projectAnnotations', () => { | |
it('renders with default projectAnnotations', () => { | ||
setProjectAnnotations([ | ||
{ | ||
testingLibraryRender: render, | ||
parameters: { injected: true }, | ||
globalTypes: { | ||
locale: { defaultValue: 'en' }, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,7 @@ | |
import type Button from './Button.svelte'; | ||
import { composeStories, composeStory, setProjectAnnotations } from '../../portable-stories'; | ||
|
||
setProjectAnnotations({ testingLibraryRender: render }); | ||
setProjectAnnotations([]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Ensure that removing |
||
|
||
// example with composeStories, returns an object with all stories composed with args/decorators | ||
const { CSF3Primary, LoaderStory } = composeStories(stories); | ||
|
@@ -82,7 +82,6 @@ | |
globalTypes: { | ||
locale: { defaultValue: 'en' }, | ||
}, | ||
testingLibraryRender: render, | ||
}, | ||
]); | ||
const WithEnglishText = composeStory(stories.CSF2StoryWithLocale, stories.default); | ||
|
@@ -172,5 +171,5 @@ | |
it.each(testCases)('Renders %s story', async (_storyName, Story) => { | ||
if (_storyName === 'CSF2StoryWithLocale') return; | ||
await Story.run(); | ||
expect(document.body).toMatchSnapshot(); | ||
Check failure on line 174 in code/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts GitHub Actions / Core Unit Tests, windows-latestsrc/__test__/composeStories/portable-stories.test.ts > Renders CSF2Secondary story
Check failure on line 174 in code/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts GitHub Actions / Core Unit Tests, windows-latestsrc/__test__/composeStories/portable-stories.test.ts > Renders CSF2StoryWithParamsAndDecorator story
Check failure on line 174 in code/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts GitHub Actions / Core Unit Tests, windows-latestsrc/__test__/composeStories/portable-stories.test.ts > Renders NewStory story
Check failure on line 174 in code/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts GitHub Actions / Core Unit Tests, windows-latestsrc/__test__/composeStories/portable-stories.test.ts > Renders CSF3Primary story
Check failure on line 174 in code/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts GitHub Actions / Core Unit Tests, windows-latestsrc/__test__/composeStories/portable-stories.test.ts > Renders CSF3Button story
Check failure on line 174 in code/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts GitHub Actions / Core Unit Tests, windows-latestsrc/__test__/composeStories/portable-stories.test.ts > Renders CSF3ButtonWithRender story
Check failure on line 174 in code/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts GitHub Actions / Core Unit Tests, windows-latestsrc/__test__/composeStories/portable-stories.test.ts > Renders CSF3InputFieldFilled story
Check failure on line 174 in code/renderers/svelte/src/__test__/composeStories/portable-stories.test.ts GitHub Actions / Core Unit Tests, windows-latestsrc/__test__/composeStories/portable-stories.test.ts > Renders LoaderStory story
|
||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,9 +65,7 @@ export const INTERNAL_DEFAULT_PROJECT_ANNOTATIONS: ProjectAnnotations<SvelteRend | |
...svelteProjectAnnotations, | ||
renderToCanvas: (renderContext, canvasElement) => { | ||
if (renderContext.storyContext.testingLibraryRender == null) { | ||
throw new TestingLibraryMustBeConfiguredError(); | ||
// Enable for 8.3 | ||
// return svelteProjectAnnotations.renderToCanvas(renderContext, canvasElement); | ||
return svelteProjectAnnotations.renderToCanvas(renderContext, canvasElement); | ||
Comment on lines
68
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. check: Consider logging a warning or error if |
||
} | ||
const { | ||
storyFn, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,7 @@ | |
import type Button from './Button.vue'; | ||
import { composeStories, composeStory, setProjectAnnotations } from '../../portable-stories'; | ||
|
||
setProjectAnnotations({ testingLibraryRender: render }); | ||
setProjectAnnotations([]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Ensure that removing |
||
|
||
// example with composeStories, returns an object with all stories composed with args/decorators | ||
const { CSF3Primary, LoaderStory } = composeStories(stories); | ||
|
@@ -60,7 +60,6 @@ | |
it('renders with default projectAnnotations', () => { | ||
setProjectAnnotations([ | ||
{ | ||
testingLibraryRender: render, | ||
parameters: { injected: true }, | ||
globalTypes: { | ||
locale: { defaultValue: 'en' }, | ||
|
@@ -147,5 +146,5 @@ | |
if (typeof Story === 'string' || _storyName === 'CSF2StoryWithLocale') return; | ||
await Story.run(); | ||
await new Promise((resolve) => setTimeout(resolve, 0)); | ||
expect(document.body).toMatchSnapshot(); | ||
Check failure on line 149 in code/renderers/vue3/src/__tests__/composeStories/portable-stories.test.ts GitHub Actions / Core Unit Tests, windows-latestsrc/__tests__/composeStories/portable-stories.test.ts > Renders CSF2Secondary story
Check failure on line 149 in code/renderers/vue3/src/__tests__/composeStories/portable-stories.test.ts GitHub Actions / Core Unit Tests, windows-latestsrc/__tests__/composeStories/portable-stories.test.ts > Renders CSF2StoryWithParamsAndDecorator story
|
||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,9 +55,7 @@ export const vueProjectAnnotations: ProjectAnnotations<VueRenderer> = { | |
...defaultProjectAnnotations, | ||
renderToCanvas: (renderContext, canvasElement) => { | ||
if (renderContext.storyContext.testingLibraryRender == null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider logging a warning when |
||
throw new TestingLibraryMustBeConfiguredError(); | ||
// Enable for 8.3 | ||
// return defaultProjectAnnotations.renderToCanvas(renderContext, canvasElement); | ||
return defaultProjectAnnotations.renderToCanvas(renderContext, canvasElement); | ||
} | ||
const { | ||
storyFn, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
```tsx filename="setupTest.ts" renderer="react" language="ts" | ||
import { beforeAll } from '@jest/globals'; | ||
import { render as testingLibraryRender } from '@testing-library/react'; | ||
import { setProjectAnnotations } from '@storybook/react'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Ensure that the internal |
||
// 👇 Import the exported annotations, if any, from the addons you're using; otherwise remove this | ||
import * as addonAnnotations from 'my-addon/preview'; | ||
|
@@ -9,8 +8,6 @@ import * as previewAnnotations from './.storybook/preview'; | |
const annotations = setProjectAnnotations([ | ||
previewAnnotations, | ||
addonAnnotations, | ||
// You MUST provide this option to use portable stories with vitest | ||
{ testingLibraryRender }, | ||
]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. check: Verify that all necessary annotations are included to avoid missing configurations. |
||
|
||
// Supports beforeAll hook from Storybook | ||
|
@@ -19,7 +16,6 @@ beforeAll(annotations.beforeAll); | |
|
||
```tsx filename="setupTest.ts" renderer="vue" language="ts" | ||
import { beforeAll } from '@jest/globals'; | ||
import { render as testingLibraryRender } from '@testing-library/vue'; | ||
import { setProjectAnnotations } from '@storybook/vue3'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Ensure that the internal |
||
// 👇 Import the exported annotations, if any, from the addons you're using; otherwise remove this | ||
import * as addonAnnotations from 'my-addon/preview'; | ||
|
@@ -28,8 +24,6 @@ import * as previewAnnotations from './.storybook/preview'; | |
const annotations = setProjectAnnotations([ | ||
previewAnnotations, | ||
addonAnnotations, | ||
// You MUST provide this option to use portable stories with vitest | ||
{ testingLibraryRender }, | ||
]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. check: Verify that all necessary annotations are included to avoid missing configurations. |
||
|
||
// Supports beforeAll hook from Storybook | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,8 +8,6 @@ import * as previewAnnotations from './.storybook/preview'; | |
const annotations = setProjectAnnotations([ | ||
previewAnnotations, | ||
addonAnnotations, | ||
// You MUST provide this option to use portable stories with vitest | ||
{ testingLibraryRender }, | ||
]); | ||
Comment on lines
8
to
11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Ensure that removing |
||
|
||
// Supports beforeAll hook from Storybook | ||
|
@@ -26,8 +24,6 @@ import * as previewAnnotations from './.storybook/preview'; | |
const annotations = setProjectAnnotations([ | ||
previewAnnotations, | ||
addonAnnotations, | ||
// You MUST provide this option to use portable stories with vitest | ||
{ testingLibraryRender }, | ||
]); | ||
|
||
// Supports beforeAll hook from Storybook | ||
|
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.
style: Ensure the internal
renderToCanvas
method is robust and can handle all scenarios previously managed bytestingLibraryRender
.