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: Normalize parameters in store/channel #10373

Merged
merged 29 commits into from
Apr 29, 2020
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
9ae73c7
Store parameters in the store in a normalized way
tmeasday Apr 10, 2020
e92b44e
Merge remote-tracking branch 'origin/next' into 10361-normalize-param…
tmeasday Apr 21, 2020
7307b76
Fix bad merge
tmeasday Apr 21, 2020
7adf8c0
Fix story sort tests
tmeasday Apr 21, 2020
4a87d5a
Combine parameters in `fromId()`
tmeasday Apr 21, 2020
198a601
We'll always define `parameters.argTypes`
tmeasday Apr 21, 2020
603dfc5
Normalize parameters in the `SET_STORIES` event
tmeasday Apr 21, 2020
bd9dd9f
Rename `SET_STORIES` to `SET_STORY_STORE_DATA`
tmeasday Apr 22, 2020
9b91f38
Pick up in the manager
tmeasday Apr 22, 2020
01867f9
Duplicate types and combineParameters
tmeasday Apr 22, 2020
1bdd48b
Bugfix
tmeasday Apr 22, 2020
febec0f
Merge branch 'next' into 10361-normalize-parameters
ndelangen Apr 23, 2020
cb28194
ADD rest data to ref
ndelangen Apr 23, 2020
63682a2
Explicitly don't allow setting storySort any other way
tmeasday Apr 28, 2020
c0cdd3f
Export `combineParameters` and document in migrations
tmeasday Apr 28, 2020
de6d52e
Remove spurious log
tmeasday Apr 28, 2020
0c6cbd6
Revert "Rename `SET_STORIES` to `SET_STORY_STORE_DATA`"
tmeasday Apr 28, 2020
fecf1d9
More spurious logs
tmeasday Apr 28, 2020
385fe81
Update to use a single SET_STORIES event
tmeasday Apr 28, 2020
afdfd04
Merge branch 'next' into 10361-normalize-parameters
shilman Apr 29, 2020
12a2853
Fix typo
shilman Apr 29, 2020
884bb15
SET_STORY_STORE_DATA => SET_STORIES
shilman Apr 29, 2020
27fadc6
Normalized parameters: Fix failing test
shilman Apr 29, 2020
8390c62
Fix inclusion logic
shilman Apr 29, 2020
03425f6
Fix deepscan
shilman Apr 29, 2020
c0f8fb2
Merge branch 'next' into 10361-normalize-parameters
shilman Apr 29, 2020
241d06c
Ensure we set `parameters.argTypes` in the preview
tmeasday Apr 29, 2020
a1b9372
Fix failing tests
shilman Apr 29, 2020
e2d54f6
Update snapshots
shilman Apr 29, 2020
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
13 changes: 11 additions & 2 deletions lib/api/src/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@ import React, {
} from 'react';

import {
SET_STORIES,
STORY_CHANGED,
SHARED_STATE_CHANGED,
SHARED_STATE_SET,
SET_STORY_STORE_DATA,
} from '@storybook/core-events';
import { RenderData as RouterData } from '@storybook/router';
import { Listener } from '@storybook/channels';
Expand Down Expand Up @@ -93,9 +93,14 @@ export type ManagerProviderProps = RouterData &
children: ReactNode | ((props: Combo) => ReactNode);
};

// These types are duplicated in addons.
export type StoryId = string;
export type StoryKind = string;

export interface Args {
[key: string]: any;
}

export interface ArgType {
name?: string;
description?: string;
Expand All @@ -107,6 +112,10 @@ export interface ArgTypes {
[key: string]: ArgType;
}

export interface Parameters {
[key: string]: any;
}

export type ModuleFn = (m: ModuleArgs) => Module;

interface Module {
Expand Down Expand Up @@ -350,7 +359,7 @@ export function useSharedState<S>(stateId: string, defaultState?: S) {
[`${SHARED_STATE_SET}-client-${stateId}`]: (s: S) => setState(s),
};
const stateInitializationHandlers = {
[SET_STORIES]: () => {
[SET_STORY_STORE_DATA]: () => {
Copy link
Member

Choose a reason for hiding this comment

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

I thought we were using SET_STORIES instead of SET_STORY_STORE_DATA? Looks like it only got partially reverted?

if (addonStateCache[stateId]) {
// this happens when HMR
setState(addonStateCache[stateId]);
Expand Down
41 changes: 37 additions & 4 deletions lib/api/src/lib/stories.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import deprecate from 'util-deprecate';
import dedent from 'ts-dedent';
import { sanitize, parseKind } from '@storybook/csf';
import { mapValues, mergeWith } from 'lodash';

import { Args } from '../index';
import { StoryId, StoryKind, Args, Parameters } from '../index';
import merge from './merge';
import { Provider } from '../modules/provider';

export type StoryId = string;
export { StoryId };

export interface Root {
id: StoryId;
Expand Down Expand Up @@ -46,7 +47,7 @@ export interface Story {
depth: number;
parent: StoryId;
name: string;
kind: string;
kind: StoryKind;
refId?: string;
children?: StoryId[];
isComponent: boolean;
Expand All @@ -70,7 +71,7 @@ export interface StoryInput {
id: StoryId;
name: string;
refId?: string;
kind: string;
kind: StoryKind;
children: string[];
parameters: {
fileName: string;
Expand Down Expand Up @@ -99,6 +100,14 @@ export interface StoriesRaw {
[id: string]: StoryInput;
}

export interface StoryStoreData {
globalParameters: Parameters;
kindParameters: {
[kind: string]: Parameters;
};
stories: StoriesRaw;
}

const warnUsingHierarchySeparatorsAndShowRoots = deprecate(
() => {},
dedent`
Expand Down Expand Up @@ -131,6 +140,30 @@ const toGroup = (name: string) => ({
id: toKey(name),
});

// This is duplicated from @storybook/client-api for the reasons mentioned in lib-addons/types.js
const combineParameters = (...parameterSets: Parameters[]) =>
mergeWith({}, ...parameterSets, (objValue: any, srcValue: any) => {
// Treat arrays as scalars:
if (Array.isArray(srcValue)) return srcValue;

return undefined;
});

export const denormalizeStoryParameters = ({
globalParameters,
kindParameters,
stories,
}: StoryStoreData): StoriesRaw => {
return mapValues(stories, (storyData) => ({
...storyData,
parameters: combineParameters(
globalParameters,
kindParameters[storyData.kind],
(storyData.parameters as unknown) as Parameters
),
}));
};

export const transformStoriesRawToStoriesHash = (
input: StoriesRaw,
base: StoriesHash,
Expand Down
48 changes: 38 additions & 10 deletions lib/api/src/modules/stories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,24 @@ import {
UPDATE_STORY_ARGS,
STORY_ARGS_UPDATED,
STORY_CHANGED,
SET_STORIES,
SELECT_STORY,
SET_STORY_STORE_DATA,
LEGACY_SET_STORIES,
} from '@storybook/core-events';

import { logger } from '@storybook/client-logger';
import {
denormalizeStoryParameters,
transformStoriesRawToStoriesHash,
StoriesHash,
Story,
Group,
StoriesRaw,
StoryStoreData,
StoryId,
isStory,
Root,
isRoot,
StoriesRaw,
} from '../lib/stories';

import { Args, ModuleFn } from '../index';
Expand Down Expand Up @@ -299,27 +302,52 @@ export const init: ModuleFn = ({
}
});

fullAPI.on(SET_STORIES, function handleSetStories(data: { stories: StoriesRaw }) {
fullAPI.on(SET_STORY_STORE_DATA, function handleSetStories(data: StoryStoreData) {
// the event originates from an iframe, event.source is the iframe's location origin + pathname
const { storyId } = store.getState();
const { source }: { source: string } = this;
const sourceType = getSourceType(source);

const stories = denormalizeStoryParameters(data);

switch (sourceType) {
// if it's a local source, we do nothing special
case 'local': {
fullAPI.setStories(data.stories);
const options = storyId
? fullAPI.getParameters(storyId, 'options')
: fullAPI.getParameters(Object.keys(data.stories)[0], 'options');
fullAPI.setOptions(options);
fullAPI.setStories(stories);

fullAPI.setOptions((data as StoryStoreData).globalParameters.options);
break;
}

// if it's a ref, we need to map the incoming stories to a prefixed version, so it cannot conflict with others
case 'external': {
const ref = fullAPI.findRef(source);
fullAPI.setRef(ref.id, { ...ref, ...data }, true);
fullAPI.setRef(ref.id, { ...ref, stories }, true);
break;
}

// if we couldn't find the source, something risky happened, we ignore the input, and log a warning
default: {
logger.warn('received a SET_STORY_STORE_DATA frame that was not configured as a ref');
break;
}
}
});

fullAPI.on(LEGACY_SET_STORIES, function handleSetStories({ stories }: { stories: StoriesRaw }) {
// the event originates from an iframe, event.source is the iframe's location origin + pathname
const { source }: { source: string } = this;
const sourceType = getSourceType(source);

switch (sourceType) {
// We shouldn't get the legacy event from the local source
case 'local': {
throw new Error('Unexpected legacy SET_STORIES event from local source');
}

// Basically the same as `SET_STORIES_DATA` above, except the parameters are denormalized
case 'external': {
const ref = fullAPI.findRef(source);
fullAPI.setRef(ref.id, { ...ref, stories }, true);
break;
}

Expand Down
121 changes: 120 additions & 1 deletion lib/api/src/tests/stories.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
import EventEmitter from 'event-emitter';
import { STORY_ARGS_UPDATED, UPDATE_STORY_ARGS } from '@storybook/core-events';
import {
STORY_ARGS_UPDATED,
UPDATE_STORY_ARGS,
SET_STORY_STORE_DATA,
LEGACY_SET_STORIES,
} from '@storybook/core-events';

import { init as initStories } from '../modules/stories';

Expand Down Expand Up @@ -791,4 +796,118 @@ describe('stories API', () => {
});
});
});
describe('SET_STORY_STORE_DATA event', () => {
it('normalizes parameters and calls setStories for local stories', () => {
const fullAPI = { on: jest.fn(), setStories: jest.fn(), setOptions: jest.fn() };
const navigate = jest.fn();
const store = createMockStore();

const { init } = initStories({ store, navigate, provider, fullAPI });
init();

const onSetStoryStoreData = fullAPI.on.mock.calls.find(
([event]) => event === SET_STORY_STORE_DATA
)[1];
Copy link
Member Author

@tmeasday tmeasday Apr 22, 2020

Choose a reason for hiding this comment

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

Is there a better way to do this for tests @ndelangen?


const storyStoreData = {
globalParameters: { global: 'global' },
kindParameters: { a: { kind: 'kind' } },
stories: { 'a--1': { kind: 'a', parameters: { story: 'story' } } },
};
onSetStoryStoreData.call({ source: 'http://localhost' }, storyStoreData);

expect(fullAPI.setStories).toHaveBeenCalledWith({
'a--1': { kind: 'a', parameters: { global: 'global', kind: 'kind', story: 'story' } },
});
});

it('normalizes parameters and calls setRef for external stories', () => {
const fullAPI = {
on: jest.fn(),
findRef: jest.fn().mockReturnValue({ id: 'ref' }),
setRef: jest.fn(),
};
const navigate = jest.fn();
const store = createMockStore();

const { init } = initStories({ store, navigate, provider, fullAPI });
init();

const onSetStoryStoreData = fullAPI.on.mock.calls.find(
([event]) => event === SET_STORY_STORE_DATA
)[1];

const storyStoreData = {
globalParameters: { global: 'global' },
kindParameters: { a: { kind: 'kind' } },
stories: { 'a--1': { kind: 'a', parameters: { story: 'story' } } },
};
onSetStoryStoreData.call({ source: 'http://external' }, storyStoreData);

expect(fullAPI.setRef).toHaveBeenCalledWith(
'ref',
{
id: 'ref',
stories: {
'a--1': { kind: 'a', parameters: { global: 'global', kind: 'kind', story: 'story' } },
},
},
true
);
});

it('calls setOptions with global options parameters', () => {
const fullAPI = { on: jest.fn(), setStories: jest.fn(), setOptions: jest.fn() };
const navigate = jest.fn();
const store = createMockStore();

const { init } = initStories({ store, navigate, provider, fullAPI });
init();

const onSetStoryStoreData = fullAPI.on.mock.calls.find(
([event]) => event === SET_STORY_STORE_DATA
)[1];

const storyStoreData = {
globalParameters: { options: 'options' },
kindParameters: { a: { options: 'should-be-ignored' } },
stories: { 'a--1': { kind: 'a', parameters: { options: 'should-be-ignored-also' } } },
};
onSetStoryStoreData.call({ source: 'http://localhost' }, storyStoreData);

expect(fullAPI.setOptions).toHaveBeenCalledWith('options');
});
});
describe('LEGACY_SET_STORIES event', () => {
it('calls setRef with stories', () => {
const fullAPI = {
on: jest.fn(),
findRef: jest.fn().mockReturnValue({ id: 'ref' }),
setRef: jest.fn(),
};
const navigate = jest.fn();
const store = createMockStore();

const { init } = initStories({ store, navigate, provider, fullAPI });
init();

const onSetStories = fullAPI.on.mock.calls.find(([event]) => event === LEGACY_SET_STORIES)[1];

const storiesData = {
stories: { 'a--1': {} },
};
onSetStories.call({ source: 'http://external' }, storiesData);

expect(fullAPI.setRef).toHaveBeenCalledWith(
'ref',
{
id: 'ref',
stories: {
'a--1': {},
},
},
true
);
});
});
});
6 changes: 4 additions & 2 deletions lib/client-api/src/client_api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,7 @@ describe('preview.client_api', () => {
storyStore.finishConfiguring();

let [event, args] = mockChannelEmit.mock.calls[0];
expect(event).toEqual(Events.SET_STORIES);
expect(event).toEqual(Events.SET_STORY_STORE_DATA);
expect(Object.values(args.stories as [{ kind: string }]).map((v) => v.kind)).toEqual([
'kind0',
'kind1',
Expand All @@ -544,7 +544,7 @@ describe('preview.client_api', () => {
// eslint-disable-next-line prefer-destructuring
[event, args] = mockChannelEmit.mock.calls[0];

expect(event).toEqual(Events.SET_STORIES);
expect(event).toEqual(Events.SET_STORY_STORE_DATA);
expect(Object.values(args.stories as [{ kind: string }]).map((v) => v.kind)).toEqual([
'kind0',
'kind1',
Expand Down Expand Up @@ -601,6 +601,7 @@ describe('preview.client_api', () => {
b: 'kind',
c: 'story',
fileName: expect.any(String),
argTypes: {},
});
});

Expand Down Expand Up @@ -653,6 +654,7 @@ describe('preview.client_api', () => {
},
},
fileName: expect.any(String),
argTypes: {},
});
});
});
Expand Down
Loading