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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSF: Allow overridding globals at the story level #26654

Draft
wants to merge 8 commits into
base: next
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions code/addons/toolbars/package.json
Expand Up @@ -56,6 +56,7 @@
"@storybook/manager-api": "workspace:*",
"@storybook/preview-api": "workspace:*",
"@storybook/theming": "workspace:*",
"dequal": "^2.0.2",
"react": "^18.2.0",
"react-dom": "^18.2.0",
"typescript": "^5.3.2"
Expand Down
9 changes: 8 additions & 1 deletion code/addons/toolbars/src/components/ToolbarMenuButton.tsx
Expand Up @@ -4,6 +4,7 @@ import { Icons, IconButton, type IconsProps } from '@storybook/components';

interface ToolbarMenuButtonProps {
active: boolean;
disabled?: boolean;
title: string;
icon?: IconsProps['icon'];
description: string;
Expand All @@ -16,13 +17,19 @@ interface ToolbarMenuButtonProps {

export const ToolbarMenuButton: FC<ToolbarMenuButtonProps> = ({
active,
disabled,
title,
icon,
description,
onClick,
}) => {
return (
<IconButton active={active} title={description} onClick={onClick}>
<IconButton
active={active}
title={description}
disabled={disabled}
onClick={disabled ? () => {} : onClick}
>
{icon && <Icons icon={icon} __suppressDeprecationWarning={true} />}
{title ? `\xa0${title}` : null}
</IconButton>
Expand Down
7 changes: 5 additions & 2 deletions code/addons/toolbars/src/components/ToolbarMenuList.tsx
Expand Up @@ -2,6 +2,7 @@ import type { FC } from 'react';
import React, { useState, useCallback } from 'react';
import { useGlobals } from '@storybook/manager-api';
import { WithTooltip, TooltipLinkList } from '@storybook/components';
import { dequal as deepEqual } from 'dequal';
import { ToolbarMenuButton } from './ToolbarMenuButton';
import type { WithKeyboardCycleProps } from '../hoc/withKeyboardCycle';
import { withKeyboardCycle } from '../hoc/withKeyboardCycle';
Expand All @@ -18,11 +19,12 @@ export const ToolbarMenuList: FC<ToolbarMenuListProps> = withKeyboardCycle(
description,
toolbar: { icon: _icon, items, title: _title, preventDynamicIcon, dynamicTitle },
}) => {
const [globals, updateGlobals] = useGlobals();
const [globals, updateGlobals, userGlobals] = useGlobals();
const [isTooltipVisible, setIsTooltipVisible] = useState(false);

const currentValue = globals[id];
const hasGlobalValue = !!currentValue;
const isOverridden = !deepEqual(currentValue, userGlobals[id]);
Copy link
Contributor

Choose a reason for hiding this comment

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

a deep equal check might be problematic for cases where functions or class instances are assigned to global values, leading to potential circular references.

Why can't we check whether userGlobals[id] is present? This matches my expectation of an "override". Otherwise, we should also rename the variable to something like isDeepEqual.

Suggested change
const isOverridden = !deepEqual(currentValue, userGlobals[id]);
const isOverridden = id in userGlobals;

Copy link
Member Author

@tmeasday tmeasday Mar 29, 2024

Choose a reason for hiding this comment

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

@valentinpalkovic:

a deep equal check might be problematic for cases where functions or class instances are assigned to global values

globals need to be serializable to go over the channel, so I don't think your examples are possible. But there may be others we should consider.

Why can't we check whether userGlobals[id] is present?

So I think you have this backwards (this might be a sign that the naming is bad) - userGlobals are the globals the user has specified via clicking toolbars etc. globals = { ...userGlobals, ...globalOverrides }. So this is how we check if a globalOverride is set for a key (does globals[key] == userGlobals[key]).

Now I think about it, maybe we've made it difficult to do the thing that you generally would want to do (know if a value has been overrdden). Perhaps instead we should set { globals, globalOverrides } on the event, or even { globals, globalOverrides, userGlobals }?

WDYT @shilman? The downside is quite a bit of redundancy on the data sent on the channel.

let icon = _icon;
let title = _title;

Expand All @@ -42,7 +44,7 @@ export const ToolbarMenuList: FC<ToolbarMenuListProps> = withKeyboardCycle(
(value: string | undefined) => {
updateGlobals({ [id]: value });
},
[currentValue, updateGlobals]
[id, updateGlobals]
);

return (
Expand Down Expand Up @@ -79,6 +81,7 @@ export const ToolbarMenuList: FC<ToolbarMenuListProps> = withKeyboardCycle(
>
<ToolbarMenuButton
active={isTooltipVisible || hasGlobalValue}
disabled={isOverridden}
Copy link
Member

Choose a reason for hiding this comment

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

Readonly might be a better UX. See this recent PR

#26577

Copy link
Member Author

Choose a reason for hiding this comment

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

Can we leave it to someone else (w/ @cdedreuille's help) to create a readonly version of the TooltipLinkList & ToolbarMenuButton? They doesn't appear to exist at the moment.

description={description || ''}
icon={icon}
title={title || ''}
Expand Down
6 changes: 6 additions & 0 deletions code/addons/toolbars/template/stories/globals.stories.ts
Expand Up @@ -31,3 +31,9 @@ export default {
};

export const Basic = {};

export const Override = {
globalOverrides: {
locale: 'kr',
},
};
9 changes: 7 additions & 2 deletions code/lib/manager-api/src/index.tsx
Expand Up @@ -28,6 +28,7 @@ import type {
API_StoryEntry,
Parameters,
StoryId,
Globals,
} from '@storybook/types';

import {
Expand Down Expand Up @@ -495,9 +496,13 @@ export function useArgs(): [Args, (newArgs: Args) => void, (argNames?: string[])
return [args!, updateArgs, resetArgs];
}

export function useGlobals(): [Args, (newGlobals: Args) => void] {
export function useGlobals(): [
globals: Globals,
updateGlobals: (newGlobals: Globals) => void,
userGlobals: Globals,
] {
const api = useStorybookApi();
return [api.getGlobals(), api.updateGlobals];
return [api.getGlobals(), api.updateGlobals, api.getUserGlobals()];
}

export function useGlobalTypes(): ArgTypes {
Expand Down
39 changes: 27 additions & 12 deletions code/lib/manager-api/src/modules/globals.ts
Expand Up @@ -9,23 +9,28 @@ import { getEventMetadata } from '../lib/events';

export interface SubState {
globals?: Globals;
userGlobals?: Globals;
globalTypes?: GlobalTypes;
}

export interface SubAPI {
/**
* Returns the current global data object.
* @returns {Globals} The current global data object.
* Returns the current globals, including overrides.
* @returns {Globals} The current globals.
*/
getGlobals: () => Globals;
/**
* Returns the current global types object.
* @returns {GlobalTypes} The current global types object.
* Returns the current globals, as set by the user (a story may have override values)
* @returns {Globals} The current user globals.
*/
getUserGlobals: () => Globals /**
* Returns the globalTypes, as defined at the project level.
* @returns {GlobalTypes} The globalTypes.
*/;
getGlobalTypes: () => GlobalTypes;
/**
* Updates the current global data object with the provided new global data object.
* @param {Globals} newGlobals - The new global data object to update with.
* Updates the current globals with the provided new globals.
* @param {Globals} newGlobals - The new globals to update with.
* @returns {void}
*/
updateGlobals: (newGlobals: Globals) => void;
Expand All @@ -36,6 +41,9 @@ export const init: ModuleFn<SubAPI, SubState> = ({ store, fullAPI, provider }) =
getGlobals() {
return store.getState().globals as Globals;
},
getUserGlobals() {
return store.getState().userGlobals as Globals;
},
getGlobalTypes() {
return store.getState().globalTypes as GlobalTypes;
},
Expand All @@ -52,22 +60,29 @@ export const init: ModuleFn<SubAPI, SubState> = ({ store, fullAPI, provider }) =

const state: SubState = {
globals: {},
userGlobals: {},
globalTypes: {},
};
const updateGlobals = (globals: Globals) => {
const currentGlobals = store.getState()?.globals;
const updateGlobals = ({ globals, userGlobals }: { globals: Globals; userGlobals: Globals }) => {
const { globals: currentGlobals, userGlobals: currentUserGlobals } = store.getState();
if (!deepEqual(globals, currentGlobals)) {
store.setState({ globals });
}
if (!deepEqual(userGlobals, currentUserGlobals)) {
store.setState({ userGlobals });
}
};

provider.channel?.on(
GLOBALS_UPDATED,
function handleGlobalsUpdated(this: any, { globals }: { globals: Globals }) {
function handleGlobalsUpdated(
this: any,
{ globals, userGlobals }: { globals: Globals; userGlobals: Globals }
) {
const { ref } = getEventMetadata(this, fullAPI)!;

if (!ref) {
updateGlobals(globals);
updateGlobals({ globals, userGlobals });
} else {
logger.warn(
'received a GLOBALS_UPDATED from a non-local ref. This is not currently supported.'
Expand All @@ -79,12 +94,12 @@ export const init: ModuleFn<SubAPI, SubState> = ({ store, fullAPI, provider }) =
// Emitted by the preview on initialization
provider.channel?.on(
SET_GLOBALS,
function handleSetStories(this: any, { globals, globalTypes }: SetGlobalsPayload) {
function handleSetGlobals(this: any, { globals, globalTypes }: SetGlobalsPayload) {
const { ref } = getEventMetadata(this, fullAPI)!;
const currentGlobals = store.getState()?.globals;

if (!ref) {
store.setState({ globals, globalTypes });
store.setState({ globals, userGlobals: globals, globalTypes });
} else if (Object.keys(globals).length > 0) {
logger.warn('received globals from a non-local ref. This is not currently supported.');
}
Expand Down
2 changes: 1 addition & 1 deletion code/lib/manager-api/src/modules/url.ts
Expand Up @@ -234,7 +234,7 @@ export const init: ModuleFn<SubAPI, SubState> = (moduleArgs) => {
}
});

provider.channel?.on(GLOBALS_UPDATED, ({ globals, initialGlobals }: any) => {
provider.channel?.on(GLOBALS_UPDATED, ({ userGlobals: globals, initialGlobals }: any) => {
const { path, queryParams } = api.getUrlState();
const globalsString = buildArgsParam(initialGlobals, globals);
navigateTo(path, { ...queryParams, globals: globalsString }, { replace: true });
Expand Down
40 changes: 31 additions & 9 deletions code/lib/preview-api/src/modules/preview-web/Preview.tsx
Expand Up @@ -199,7 +199,7 @@ export class Preview<TRenderer extends Renderer> {
throw new CalledPreviewMethodBeforeInitializationError({ methodName: 'emitGlobals' });

const payload: SetGlobalsPayload = {
globals: this.storyStoreValue.globals.get() || {},
globals: this.storyStoreValue.userGlobals.get() || {},
globalTypes: this.storyStoreValue.projectAnnotations.globalTypes || {},
};
this.channel.emit(SET_GLOBALS, payload);
Expand Down Expand Up @@ -265,17 +265,39 @@ export class Preview<TRenderer extends Renderer> {
await this.storyStoreValue.onStoriesChanged({ importFn, storyIndex });
}

async onUpdateGlobals({ globals }: { globals: Globals }) {
if (!this.storyStoreValue)
async onUpdateGlobals({
globals: updatedGlobals,
currentStory,
}: {
globals: Globals;
currentStory?: PreparedStory<TRenderer>;
}) {
if (!this.storyStoreValue) {
throw new CalledPreviewMethodBeforeInitializationError({ methodName: 'onUpdateGlobals' });
this.storyStoreValue.globals.update(globals);
}

await Promise.all(this.storyRenders.map((r) => r.rerender()));
this.storyStoreValue.userGlobals.update(updatedGlobals);

this.channel.emit(GLOBALS_UPDATED, {
globals: this.storyStoreValue.globals.get(),
initialGlobals: this.storyStoreValue.globals.initialGlobals,
});
if (currentStory) {
const { initialGlobals, userGlobals, globals } =
this.storyStoreValue.getStoryContext(currentStory);
this.channel.emit(GLOBALS_UPDATED, {
globals,
userGlobals,
initialGlobals,
});
} else {
// If there is no known selected story (e.g. if we are in docs mode), the userGlobals
// are not overridden.
const { initialGlobals, globals } = this.storyStoreValue.userGlobals;
this.channel.emit(GLOBALS_UPDATED, {
globals,
userGlobals: globals,
initialGlobals,
});
}

await Promise.all(this.storyRenders.map((r) => r.rerender()));
}

async onUpdateArgs({ storyId, updatedArgs }: { storyId: StoryId; updatedArgs: Args }) {
Expand Down
12 changes: 7 additions & 5 deletions code/lib/preview-api/src/modules/preview-web/PreviewWeb.test.ts
Expand Up @@ -164,7 +164,7 @@ describe('PreviewWeb', () => {

const preview = await createAndRenderPreview();

expect((preview.storyStore as StoryStore<Renderer>)!.globals.get()).toEqual({ a: 'c' });
expect((preview.storyStore as StoryStore<Renderer>)!.userGlobals.get()).toEqual({ a: 'c' });
});

it('emits the SET_GLOBALS event', async () => {
Expand Down Expand Up @@ -225,7 +225,7 @@ describe('PreviewWeb', () => {
});
await preview.ready();

expect((preview.storyStore as StoryStore<Renderer>)!.globals.get()).toEqual({ a: 'b' });
expect((preview.storyStore as StoryStore<Renderer>)!.userGlobals.get()).toEqual({ a: 'b' });
});
});

Expand Down Expand Up @@ -781,7 +781,7 @@ describe('PreviewWeb', () => {

emitter.emit(UPDATE_GLOBALS, { globals: { foo: 'bar' } });

expect((preview.storyStore as StoryStore<Renderer>)!.globals.get()).toEqual({ a: 'b' });
expect((preview.storyStore as StoryStore<Renderer>)!.userGlobals.get()).toEqual({ a: 'b' });
});

it('passes globals in context to renderToCanvas', async () => {
Expand Down Expand Up @@ -3320,7 +3320,7 @@ describe('PreviewWeb', () => {
preview.onGetProjectAnnotationsChanged({ getProjectAnnotations });
await waitForRender();

expect((preview.storyStore as StoryStore<Renderer>)!.globals.get()).toEqual({ a: 'c' });
expect((preview.storyStore as StoryStore<Renderer>)!.userGlobals.get()).toEqual({ a: 'c' });
});
});

Expand Down Expand Up @@ -3369,7 +3369,9 @@ describe('PreviewWeb', () => {
preview.onGetProjectAnnotationsChanged({ getProjectAnnotations: newGetProjectAnnotations });
await waitForRender();

expect((preview.storyStore as StoryStore<Renderer>)!.globals.get()).toEqual({ a: 'edited' });
expect((preview.storyStore as StoryStore<Renderer>)!.userGlobals.get()).toEqual({
a: 'edited',
});
});

it('emits SET_GLOBALS with new values', async () => {
Expand Down
Expand Up @@ -2,6 +2,7 @@ import invariant from 'tiny-invariant';
import {
CURRENT_STORY_WAS_SET,
DOCS_PREPARED,
GLOBALS_UPDATED,
PRELOAD_ENTRIES,
PREVIEW_KEYDOWN,
SET_CURRENT_STORY,
Expand Down Expand Up @@ -121,7 +122,7 @@ export class PreviewWithSelection<TRenderer extends Renderer> extends Preview<TR

const { globals } = this.selectionStore.selectionSpecifier || {};
if (globals) {
this.storyStoreValue.globals.updateFromPersisted(globals);
this.storyStoreValue.userGlobals.updateFromPersisted(globals);
}
this.emitGlobals();
}
Expand Down Expand Up @@ -238,7 +239,9 @@ export class PreviewWithSelection<TRenderer extends Renderer> extends Preview<TR
}

async onUpdateGlobals({ globals }: { globals: Globals }) {
super.onUpdateGlobals({ globals });
const currentStory =
(this.currentRender instanceof StoryRender && this.currentRender.story) || undefined;
super.onUpdateGlobals({ globals, currentStory });
if (
this.currentRender instanceof MdxDocsRender ||
this.currentRender instanceof CsfDocsRender
Expand Down Expand Up @@ -394,8 +397,15 @@ export class PreviewWithSelection<TRenderer extends Renderer> extends Preview<TR

if (isStoryRender(render)) {
invariant(!!render.story);
const { parameters, initialArgs, argTypes, unmappedArgs } =
this.storyStoreValue.getStoryContext(render.story);
const {
parameters,
initialArgs,
argTypes,
unmappedArgs,
initialGlobals,
userGlobals,
globals,
} = this.storyStoreValue.getStoryContext(render.story);

this.channel.emit(STORY_PREPARED, {
id: storyId,
Expand All @@ -405,6 +415,10 @@ export class PreviewWithSelection<TRenderer extends Renderer> extends Preview<TR
args: unmappedArgs,
});

// We need to update globals whenever we go in or out of an overridden story.
// As an optimization we could check if that's the case, but it seems complex and error-prone
this.channel.emit(GLOBALS_UPDATED, { userGlobals, globals, initialGlobals });

// For v6 mode / compatibility
// If the implementation changed, or args were persisted, the args may have changed,
// and the STORY_PREPARED event above may not be respected.
Expand Down
2 changes: 1 addition & 1 deletion code/lib/preview-api/src/modules/store/StoryStore.test.ts
Expand Up @@ -367,7 +367,7 @@ describe('StoryStore', () => {
const story = await store.loadStory({ storyId: 'component-one--a' });

store.args.update(story.id, { foo: 'bar' });
store.globals!.update({ a: 'c' });
store.userGlobals!.update({ a: 'c' });

expect(store.getStoryContext(story)).toMatchObject({
args: { foo: 'bar' },
Expand Down