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

Core: Add context as a property of the context (self-referencing) #28353

Merged
merged 25 commits into from
Jun 28, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
e944cc5
Consolidate loader, play and render context and add a self referencin…
kasperpeulen Jun 25, 2024
f5d8384
Implement context consolidation for portable stories
kasperpeulen Jun 26, 2024
ba61879
Add TODO for normalizeProjectAnnotations.ts
kasperpeulen Jun 26, 2024
f24a39b
Merge remote-tracking branch 'origin/next' into kasper/context-prop
kasperpeulen Jun 26, 2024
054f661
Add extra tests
kasperpeulen Jun 26, 2024
156966b
Update docs
kasperpeulen Jun 26, 2024
b765c98
Update docs
kasperpeulen Jun 26, 2024
fd11b1a
add test step_and_canvas_element_can_be_used_in_loaders_and_before_each
kasperpeulen Jun 26, 2024
1332415
Add tests for circular references in instrumenter
kasperpeulen Jun 26, 2024
b7e974c
extract maximum depth
kasperpeulen Jun 26, 2024
b9f37a5
Merge branch 'next' into kasper/context-prop
kasperpeulen Jun 26, 2024
c4f4ed5
Merge branch 'next' into kasper/context-prop
kasperpeulen Jun 26, 2024
99bc89c
Improve comment
kasperpeulen Jun 27, 2024
2fa896a
Add TODO comments
kasperpeulen Jun 27, 2024
8948af4
Fix test, cleanup and backward support to deprecated StoryStory.fromId
kasperpeulen Jun 27, 2024
a795391
Have proper support for circular references
kasperpeulen Jun 27, 2024
2a9a313
Adjust type of getStoryContext method in docs
kasperpeulen Jun 27, 2024
e15a81d
Fix type issue
kasperpeulen Jun 27, 2024
f71def6
Fix other type issue
kasperpeulen Jun 27, 2024
00af650
Fix other type issue
kasperpeulen Jun 27, 2024
9096e89
Fix missing dev dep
kasperpeulen Jun 27, 2024
053d9ee
Fix missing dev dep
kasperpeulen Jun 27, 2024
6a91865
Prettier
kasperpeulen Jun 27, 2024
fced992
Merge branch 'next' into kasper/context-prop
kasperpeulen Jun 28, 2024
ccabc98
Merge branch 'next' into kasper/context-prop
kasperpeulen Jun 28, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions code/addons/interactions/src/preview.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
import type { PlayFunction, PlayFunctionContext, StepLabel } from '@storybook/types';
import type { PlayFunction, StepLabel, StoryContext } from '@storybook/types';
import { instrument } from '@storybook/instrumenter';

export const { step: runStep } = instrument(
{
step: (label: StepLabel, play: PlayFunction, context: PlayFunctionContext<any>) =>
play(context),
// It seems like the label is unused, but the instrumenter has access to it
// The context will be bounded later in StoryRender, so that the user can write just:
// await step("label", (context) => {
// // labeled step
// });
step: (label: StepLabel, play: PlayFunction, context: StoryContext) => play(context),
},
{ intercept: true }
);
Expand Down
2 changes: 1 addition & 1 deletion code/addons/links/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
"prep": "node --loader ../../../scripts/node_modules/esbuild-register/loader.js -r ../../../scripts/node_modules/esbuild-register/register.js ../../../scripts/prepare/addon-bundle.ts"
},
"dependencies": {
"@storybook/csf": "^0.1.8",
"@storybook/csf": "0.1.10--canary.d841bb4.0",
"@storybook/global": "^5.0.0",
"ts-dedent": "^2.0.0"
},
Expand Down
2 changes: 1 addition & 1 deletion code/lib/codemod/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
"@babel/core": "^7.24.4",
"@babel/preset-env": "^7.24.4",
"@babel/types": "^7.24.0",
"@storybook/csf": "^0.1.8",
"@storybook/csf": "0.1.10--canary.d841bb4.0",
"@storybook/csf-tools": "workspace:*",
"@storybook/node-logger": "workspace:*",
"@storybook/types": "workspace:*",
Expand Down
2 changes: 1 addition & 1 deletion code/lib/core-events/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@
"prep": "node --loader ../../../scripts/node_modules/esbuild-register/loader.js -r ../../../scripts/node_modules/esbuild-register/register.js ../../../scripts/prepare/bundle.ts"
},
"dependencies": {
"@storybook/csf": "^0.1.8",
"@storybook/csf": "0.1.10--canary.d841bb4.0",
"ts-dedent": "^2.0.0"
},
"devDependencies": {
Expand Down
2 changes: 1 addition & 1 deletion code/lib/core-server/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
"@storybook/channels": "workspace:*",
"@storybook/core-common": "workspace:*",
"@storybook/core-events": "workspace:*",
"@storybook/csf": "^0.1.8",
"@storybook/csf": "0.1.10--canary.d841bb4.0",
"@storybook/csf-tools": "workspace:*",
"@storybook/docs-mdx": "3.1.0-next.0",
"@storybook/global": "^5.0.0",
Expand Down
2 changes: 1 addition & 1 deletion code/lib/csf-tools/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
"@babel/parser": "^7.24.4",
"@babel/traverse": "^7.24.1",
"@babel/types": "^7.24.0",
"@storybook/csf": "^0.1.8",
"@storybook/csf": "0.1.10--canary.d841bb4.0",
"@storybook/types": "workspace:*",
"fs-extra": "^11.1.0",
"recast": "^0.23.5",
Expand Down
9 changes: 6 additions & 3 deletions code/lib/instrumenter/src/instrumenter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -412,12 +412,14 @@ export class Instrumenter {
const { callRefsByResult, renderPhase } = this.getState(call.storyId);

// Map complex values to a JSON-serializable representation.
const serializeValues = (value: any): any => {
// We use a depth, to avoid infinite recursion of self referencing values.
kasperpeulen marked this conversation as resolved.
Show resolved Hide resolved
const serializeValues = (value: any, depth = 0): any => {
if (callRefsByResult.has(value)) {
return callRefsByResult.get(value);
}
if (value instanceof Array) {
return value.map(serializeValues);
if (depth > 10) return [];
kasperpeulen marked this conversation as resolved.
Show resolved Hide resolved
return value.map((it) => serializeValues(it, ++depth));
}
if (value instanceof Date) {
return { __date__: { value: value.toISOString() } };
Expand Down Expand Up @@ -451,8 +453,9 @@ export class Instrumenter {
return { __class__: { name: value.constructor.name } };
}
if (Object.prototype.toString.call(value) === '[object Object]') {
if (depth > 10) return {};
return Object.fromEntries(
Object.entries(value).map(([key, val]) => [key, serializeValues(val)])
Object.entries(value).map(([key, val]) => [key, serializeValues(val, ++depth)])
);
}
return value;
Expand Down
2 changes: 1 addition & 1 deletion code/lib/manager-api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
"@storybook/channels": "workspace:*",
"@storybook/client-logger": "workspace:*",
"@storybook/core-events": "workspace:*",
"@storybook/csf": "^0.1.8",
"@storybook/csf": "0.1.10--canary.d841bb4.0",
"@storybook/global": "^5.0.0",
"@storybook/icons": "^1.2.5",
"@storybook/router": "workspace:*",
Expand Down
2 changes: 1 addition & 1 deletion code/lib/preview-api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
"@storybook/channels": "workspace:*",
"@storybook/client-logger": "workspace:*",
"@storybook/core-events": "workspace:*",
"@storybook/csf": "^0.1.8",
"@storybook/csf": "0.1.10--canary.d841bb4.0",
"@storybook/global": "^5.0.0",
"@storybook/types": "workspace:*",
"@types/qs": "^6.9.5",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,15 @@ import {
STORY_THREW_EXCEPTION,
} from '@storybook/core-events';

import type { ModuleImportFn, StoryIndex, TeardownRenderToCanvas } from '@storybook/types';
import type {
ModuleImportFn,
ProjectAnnotations,
Renderer,
StoryIndex,
TeardownRenderToCanvas,
} from '@storybook/types';
import type { RenderPhase } from './render/StoryRender';
import { composeConfigs } from '../store';

export const componentOneExports = {
default: {
Expand Down Expand Up @@ -65,14 +72,18 @@ export const docsRenderer = {
unmount: vi.fn(),
};
export const teardownrenderToCanvas: Mock<[TeardownRenderToCanvas]> = vi.fn();
export const projectAnnotations = {
const rawProjectAnnotations = {
initialGlobals: { a: 'b' },
globalTypes: {},
decorators: [vi.fn((s) => s())],
render: vi.fn(),
renderToCanvas: vi.fn().mockReturnValue(teardownrenderToCanvas),
parameters: { docs: { renderer: () => docsRenderer } },
};
export const projectAnnotations = composeConfigs([
rawProjectAnnotations,
]) as ProjectAnnotations<Renderer> & typeof rawProjectAnnotations;

export const getProjectAnnotations = vi.fn(() => projectAnnotations as any);

export const storyIndex: StoryIndex = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -949,7 +949,7 @@ describe('PreviewWeb', () => {
forceRemount: true,
storyContext: expect.objectContaining({
loaded: { l: 8 }, // This is the value returned by the *first* loader call
args: { foo: 'a', new: 'arg', one: 'mapped-1' },
args: { foo: 'a', one: 'mapped-1' },
kasperpeulen marked this conversation as resolved.
Show resolved Hide resolved
}),
}),
'story-element'
Expand Down
53 changes: 24 additions & 29 deletions code/lib/preview-api/src/modules/preview-web/render/StoryRender.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import type {
StoryContextForLoaders,
StoryId,
StoryRenderOptions,
ViewMode,
} from '@storybook/types';
import type { Channel } from '@storybook/channels';
import {
Expand Down Expand Up @@ -71,7 +70,7 @@ export class StoryRender<TRenderer extends Renderer> implements Render<TRenderer
private renderToScreen: RenderToCanvas<TRenderer>,
private callbacks: RenderContextCallbacks<TRenderer>,
public id: StoryId,
public viewMode: ViewMode,
public viewMode: StoryContext['viewMode'],
public renderOptions: StoryRenderOptions = { autoplay: true, forceInitialArgs: false },
story?: PreparedStory<TRenderer>
) {
Expand Down Expand Up @@ -165,6 +164,7 @@ export class StoryRender<TRenderer extends Renderer> implements Render<TRenderer
applyBeforeEach,
unboundStoryFn,
playFunction,
runStep,
} = story;

if (forceRemount && !initial) {
Expand All @@ -180,33 +180,16 @@ export class StoryRender<TRenderer extends Renderer> implements Render<TRenderer
const abortSignal = (this.abortController as AbortController).signal;

try {
let loadedContext: Awaited<ReturnType<typeof applyLoaders>>;
await this.runPhase(abortSignal, 'loading', async () => {
loadedContext = await applyLoaders({
...this.storyContext(),
viewMode: this.viewMode,
// TODO add this to CSF
canvasElement,
} as unknown as StoryContextForLoaders<TRenderer>);
});
if (abortSignal.aborted) return;

const renderStoryContext: StoryContext<TRenderer> = {
...loadedContext!,
// By this stage, it is possible that new args/globals have been received for this story
// and we need to ensure we render it with the new values
...this.storyContext(),
tmeasday marked this conversation as resolved.
Show resolved Hide resolved
const context: StoryContext<TRenderer> = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that loaders, beforeEach, the step function, the renderer and the play function, all get the same context reference.

...(this.storyContext() as StoryContextForLoaders<TRenderer>),
viewMode: this.viewMode,
abortSignal,
// We should consider parameterizing the story types with TRenderer['canvasElement'] in the future
canvasElement: canvasElement as any,
canvasElement,
loaded: {},
step: (label, play) => runStep(label, play, context),
};

await this.runPhase(abortSignal, 'beforeEach', async () => {
const cleanupCallbacks = await applyBeforeEach(renderStoryContext);
this.store.addCleanupCallbacks(story, cleanupCallbacks);
});

if (abortSignal.aborted) return;
context.context = context;

const renderContext: RenderContext<TRenderer> = {
componentId,
Expand All @@ -226,10 +209,22 @@ export class StoryRender<TRenderer extends Renderer> implements Render<TRenderer
return this.callbacks.showException(error);
},
forceRemount: forceRemount || this.notYetRendered,
storyContext: renderStoryContext,
storyFn: () => unboundStoryFn(renderStoryContext),
storyContext: context,
storyFn: () => unboundStoryFn(context),
unboundStoryFn,
};
await this.runPhase(abortSignal, 'loading', async () => {
context.loaded = await applyLoaders(context);
});

if (abortSignal.aborted) return;

await this.runPhase(abortSignal, 'beforeEach', async () => {
const cleanupCallbacks = await applyBeforeEach(context);
this.store.addCleanupCallbacks(story, cleanupCallbacks);
});

if (abortSignal.aborted) return;

await this.runPhase(abortSignal, 'rendering', async () => {
const teardown = await this.renderToScreen(renderContext, canvasElement);
Expand All @@ -253,7 +248,7 @@ export class StoryRender<TRenderer extends Renderer> implements Render<TRenderer
this.disableKeyListeners = true;
try {
await this.runPhase(abortSignal, 'playing', async () => {
await playFunction(renderContext.storyContext);
await playFunction(context);
});
if (!ignoreUnhandledErrors && unhandledErrors.size > 0) {
await this.runPhase(abortSignal, 'errored');
Expand Down
18 changes: 12 additions & 6 deletions code/lib/preview-api/src/modules/store/StoryStore.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { prepareStory } from './csf/prepareStory';
import { processCSFFile } from './csf/processCSFFile';
import { StoryStore } from './StoryStore';
import type { HooksContext } from './hooks';
import { composeConfigs } from './csf/composeConfigs';

// Spy on prepareStory/processCSFFile
vi.mock('./csf/prepareStory', async (importOriginal) => {
Expand Down Expand Up @@ -41,12 +42,14 @@ const importFn = vi.fn(async (path) => {
return path === './src/ComponentOne.stories.js' ? componentOneExports : componentTwoExports;
});

const projectAnnotations: ProjectAnnotations<any> = {
globals: { a: 'b' },
globalTypes: { a: { type: 'string' } },
argTypes: { a: { type: 'string' } },
render: vi.fn(),
};
const projectAnnotations: ProjectAnnotations<any> = composeConfigs([
{
globals: { a: 'b' },
globalTypes: { a: { type: 'string' } },
argTypes: { a: { type: 'string' } },
render: vi.fn(),
},
]);

const storyIndex: StoryIndex = {
v: 5,
Expand Down Expand Up @@ -660,6 +663,7 @@ describe('StoryStore', () => {
"fileName": "./src/ComponentOne.stories.js",
},
"playFunction": undefined,
"runStep": [Function],
"story": "A",
"storyFn": [Function],
"subcomponents": undefined,
Expand Down Expand Up @@ -707,6 +711,7 @@ describe('StoryStore', () => {
"fileName": "./src/ComponentOne.stories.js",
},
"playFunction": undefined,
"runStep": [Function],
"story": "B",
"storyFn": [Function],
"subcomponents": undefined,
Expand Down Expand Up @@ -754,6 +759,7 @@ describe('StoryStore', () => {
"fileName": "./src/ComponentTwo.stories.js",
},
"playFunction": undefined,
"runStep": [Function],
"story": "C",
"storyFn": [Function],
"subcomponents": undefined,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import { normalizeInputTypes } from './normalizeInputTypes';
import { normalizeArrays } from './normalizeArrays';
import { combineParameters } from '../parameters';

// TODO(kasperpeulen) Consolidate this function with composeConfigs
// As composeConfigs is the real normalizer, and always run before normalizeProjectAnnotations
Comment on lines +16 to +17
Copy link
Member

Choose a reason for hiding this comment

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

I discussed this with someone recently (@shilman I think?). Alternatively we could get rid of composeConfigs and just pass ProjectAnnotations[] around -- and do the composing here.

That makes sense to me as it avoids the need for both WP + Vite to call composeConfigs at the right time.

It definitely doesn't makes sense to have both composeConfigs and normalizeProjectAnnotations.

Also I don't think these two lines belong in there:

inferArgTypes,
// inferControls technically should only run if the user is using the controls addon,
// and so should be added by a preset there. However, as it seems some code relies on controls
// annotations (in particular the angular implementation's `cleanArgsDecorator`), for backwards
// compatibility reasons, we will leave this in the store until 7.0
inferControls,

Instead, they should get added to the ProjectAnnotations[] somewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I like that. It seems better to do as little as possible JS in the webpack/vite template files.

Those 2 lines, should move to the addon-controls preview file right?

Copy link
Member

Choose a reason for hiding this comment

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

Those 2 lines, should move to the addon-controls preview file right?

Possibly, let's discuss with @shilman to make sure.

export function normalizeProjectAnnotations<TRenderer extends Renderer>({
argTypes,
globalTypes,
Expand Down Expand Up @@ -48,6 +50,6 @@ export function normalizeProjectAnnotations<TRenderer extends Renderer>({
inferControls,
],
initialGlobals: combineParameters(initialGlobals, globals),
...annotations,
...(annotations as NormalizedProjectAnnotations<TRenderer>),
};
}
25 changes: 12 additions & 13 deletions code/lib/preview-api/src/modules/store/csf/portable-stories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,12 @@ import type {
ComponentAnnotations,
LegacyStoryAnnotationsOrFn,
NamedOrDefaultProjectAnnotations,
ComposedStoryPlayFn,
ComposeStoryFn,
Store_CSFExports,
StoryContext,
Parameters,
ComposedStoryFn,
StrictArgTypes,
PlayFunctionContext,
ProjectAnnotations,
} from '@storybook/types';

Expand Down Expand Up @@ -104,18 +102,20 @@ export function composeStory<TRenderer extends Renderer = Renderer, TArgs extend
args: { ...story.initialArgs },
viewMode: 'story',
loaded: {},
abortSignal: null as unknown as AbortSignal,
canvasElement: null,
abortSignal: new AbortController().signal,
step: (label, play) => story.runStep(label, play, context),
canvasElement: globalThis?.document?.body,
context: null!,
...story,
};

context.context = context;

const playFunction = story.playFunction
? async (extraContext: Partial<PlayFunctionContext<TRenderer, TArgs>>) =>
story.playFunction!({
...context,
...extraContext,
canvasElement: extraContext?.canvasElement ?? globalThis.document?.body,
})
? async (extraContext?: Partial<StoryContext<TRenderer, Partial<TArgs>>>) => {
Object.assign(context, extraContext);
return story.playFunction!(context);
}
: undefined;

let previousCleanupsDone = false;
Expand Down Expand Up @@ -159,8 +159,7 @@ export function composeStory<TRenderer extends Renderer = Renderer, TArgs extend

previousCleanupsDone = true;

const loadedContext = await story.applyLoaders(context);
context.loaded = loadedContext.loaded;
context.loaded = await story.applyLoaders(context);

cleanups.push(
...(await story.applyBeforeEach(context))
Expand All @@ -171,7 +170,7 @@ export function composeStory<TRenderer extends Renderer = Renderer, TArgs extend
args: story.initialArgs as Partial<TArgs>,
parameters: story.parameters as Parameters,
argTypes: story.argTypes as StrictArgTypes<TArgs>,
play: playFunction as ComposedStoryPlayFn<TRenderer, TArgs> | undefined,
play: playFunction,
tags: story.tags,
}
);
Expand Down
Loading
Loading