From ff0f138157083ffa3a2f1bb99e9b9e7eb4c833a5 Mon Sep 17 00:00:00 2001 From: Michael Shilman Date: Tue, 2 Apr 2019 19:07:13 +0800 Subject: [PATCH 1/2] Refactor story_store - don't touch URL - only emit events, don't listen on them - add tests --- lib/client-api/src/story_store.js | 35 +--------- lib/client-api/src/story_store.test.js | 59 ----------------- lib/core/src/client/preview/start.js | 42 +++++++----- lib/core/src/client/preview/start.test.js | 39 ++++++++++- lib/core/src/client/preview/url.js | 39 +++++++++++ lib/core/src/client/preview/url.test.js | 79 +++++++++++++++++++++++ 6 files changed, 184 insertions(+), 109 deletions(-) create mode 100644 lib/core/src/client/preview/url.js create mode 100644 lib/core/src/client/preview/url.test.js diff --git a/lib/client-api/src/story_store.js b/lib/client-api/src/story_store.js index f6dbf7c47bd3..13d25d71dbd2 100644 --- a/lib/client-api/src/story_store.js +++ b/lib/client-api/src/story_store.js @@ -1,16 +1,11 @@ /* eslint no-underscore-dangle: 0 */ -import { history, document } from 'global'; import EventEmitter from 'eventemitter3'; -import qs from 'qs'; import memoize from 'memoizerific'; import debounce from 'lodash.debounce'; import { stripIndents } from 'common-tags'; import Events from '@storybook/core-events'; import { logger } from '@storybook/client-logger'; -import { toId } from '@storybook/router/utils'; - -import pathToId from './pathToId'; // TODO: these are copies from components/nav/lib // refactor to DRY @@ -49,9 +44,6 @@ const toExtracted = obj => return Object.assign(acc, { [key]: value }); }, {}); -const getIdFromLegacyQuery = ({ path, selectedKind, selectedStory }) => - (path && pathToId(path)) || (selectedKind && selectedStory && toId(selectedKind, selectedStory)); - export default class StoryStore extends EventEmitter { constructor(params) { super(); @@ -61,19 +53,6 @@ export default class StoryStore extends EventEmitter { this._revision = 0; this._selection = {}; this._channel = params.channel; - - this.on(Events.STORY_INIT, () => { - let storyId = this.getIdOnPath(); - if (!storyId) { - const query = qs.parse(document.location.search, { ignoreQueryPrefix: true }); - storyId = getIdFromLegacyQuery(query); - if (storyId) { - const { path, selectedKind, selectedStory, ...rest } = query; - this.setPath(storyId, rest); - } - } - this.setSelection(this.fromId(storyId)); - }); } setChannel = channel => { @@ -82,16 +61,6 @@ export default class StoryStore extends EventEmitter { // NEW apis - getIdOnPath = () => { - const { id } = qs.parse(document.location.search, { ignoreQueryPrefix: true }); - return id; - }; - - setPath = (storyId, params = {}) => { - const path = `${document.location.pathname}?${qs.stringify({ ...params, id: storyId })}`; - history.replaceState({}, '', path); - }; - fromId = id => { try { const data = this._data[id]; @@ -126,8 +95,8 @@ export default class StoryStore extends EventEmitter { ); } - setSelection = data => { - this._selection = data; + setSelection = ({ storyId }) => { + this._selection = { storyId }; setTimeout(() => this.emit(Events.STORY_RENDER), 1); }; diff --git a/lib/client-api/src/story_store.test.js b/lib/client-api/src/story_store.test.js index b68499539f24..e587e62027aa 100644 --- a/lib/client-api/src/story_store.test.js +++ b/lib/client-api/src/story_store.test.js @@ -1,25 +1,9 @@ -import { history, document } from 'global'; import createChannel from '@storybook/channel-postmessage'; -import Events from '@storybook/core-events'; import { toId } from '@storybook/router/utils'; import StoryStore from './story_store'; import { defaultDecorateStory } from './client_api'; -jest.mock('global', () => ({ - history: { replaceState: jest.fn() }, - window: { - addEventListener: jest.fn(), - }, - document: { - location: { - pathname: 'pathname', - search: '', - }, - addEventListener: jest.fn(), - }, -})); - jest.mock('@storybook/node-logger', () => ({ logger: { info: jest.fn(), @@ -148,47 +132,4 @@ describe('preview.story_store', () => { }); }); }); - - describe('setPath', () => { - it('preserves custom URL params', () => { - const store = new StoryStore({ channel }); - - store.setPath('story--id', { foo: 'bar' }); - expect(history.replaceState).toHaveBeenCalledWith({}, '', 'pathname?foo=bar&id=story--id'); - }); - }); - - describe('STORY_INIT', () => { - const storyFn = () => 0; - - it('supports path params', () => { - document.location = { - pathname: 'pathname', - search: '?path=/story/kind--story&bar=baz', - }; - const store = new StoryStore({ channel }); - store.addStory(...make('kind', 'story', storyFn)); - store.setSelection = jest.fn(); - - store.emit(Events.STORY_INIT); - expect(history.replaceState).toHaveBeenCalledWith({}, '', 'pathname?bar=baz&id=kind--story'); - expect(store.setSelection).toHaveBeenCalled(); - expect(store.setSelection.mock.calls[0][0].getDecorated()).toEqual(storyFn); - }); - - it('supports story kind/name params', () => { - document.location = { - pathname: 'pathname', - search: '?selectedKind=kind&selectedStory=story&bar=baz', - }; - const store = new StoryStore({ channel }); - store.addStory(...make('kind', 'story', storyFn)); - store.setSelection = jest.fn(); - - store.emit(Events.STORY_INIT); - expect(history.replaceState).toHaveBeenCalledWith({}, '', 'pathname?bar=baz&id=kind--story'); - expect(store.setSelection).toHaveBeenCalled(); - expect(store.setSelection.mock.calls[0][0].getDecorated()).toEqual(storyFn); - }); - }); }); diff --git a/lib/core/src/client/preview/start.js b/lib/core/src/client/preview/start.js index deaa08b67749..6593ef2824aa 100644 --- a/lib/core/src/client/preview/start.js +++ b/lib/core/src/client/preview/start.js @@ -6,6 +6,7 @@ import { toId } from '@storybook/router/utils'; import { logger } from '@storybook/client-logger'; import Events from '@storybook/core-events'; import deprecate from 'util-deprecate'; +import { initializePath, setPath } from './url'; const classes = { MAIN: 'sb-show-main', @@ -114,8 +115,19 @@ export default function start(render, { decorateStory } = {}) { const renderMain = forceRender => { const revision = storyStore.getRevision(); - const selection = storyStore.getSelection(); - const { kind, name, getDecorated, id } = selection || {}; + const { storyId } = storyStore.getSelection(); + + const data = storyStore.fromId(storyId); + + const { kind, name, getDecorated, id } = data || {}; + + const renderContext = { + ...context, + ...data, + selectedKind: kind, + selectedStory: name, + forceRender, + }; if (getDecorated) { // Render story only if selectedKind or selectedStory have changed. @@ -135,21 +147,16 @@ export default function start(render, { decorateStory } = {}) { addons.getChannel().emit(Events.STORY_CHANGED, id); } - render({ - ...context, - ...selection, - selectedKind: kind, - selectedStory: name, - forceRender, - }); + previousRevision = revision; + previousKind = kind; + previousStory = name; + + render(renderContext); addons.getChannel().emit(Events.STORY_RENDERED, id); } else { showNopreview(); addons.getChannel().emit(Events.STORY_MISSING, id); } - previousRevision = revision; - previousKind = kind; - previousStory = name; if (!forceRender) { document.documentElement.scrollTop = 0; @@ -187,10 +194,8 @@ export default function start(render, { decorateStory } = {}) { storyId = deprecatedToId(kind, name); } - const data = storyStore.fromId(storyId); - - storyStore.setSelection(data); - storyStore.setPath(storyId); + storyStore.setSelection({ storyId }); + setPath({ storyId }); }); // Handle keyboard shortcuts @@ -205,6 +210,11 @@ export default function start(render, { decorateStory } = {}) { }; } + storyStore.on(Events.STORY_INIT, () => { + const { storyId } = initializePath(); + storyStore.setSelection({ storyId }); + }); + storyStore.on(Events.STORY_RENDER, renderUI); if (typeof window !== 'undefined') { diff --git a/lib/core/src/client/preview/start.test.js b/lib/core/src/client/preview/start.test.js index 4b9270593e16..df34092ab60c 100644 --- a/lib/core/src/client/preview/start.test.js +++ b/lib/core/src/client/preview/start.test.js @@ -1,9 +1,12 @@ -import { document } from 'global'; +/* eslint-disable no-underscore-dangle */ +import { history, document } from 'global'; +import Events from '@storybook/core-events'; import start from './start'; jest.mock('@storybook/client-logger'); jest.mock('global', () => ({ + history: { replaceState: jest.fn() }, navigator: { userAgent: 'browser', platform: '' }, window: { addEventListener: jest.fn(), @@ -85,3 +88,37 @@ it('emits an error and shows error when your framework calls showError', () => { expect(render).toHaveBeenCalled(); expect(document.body.classList.add).toHaveBeenCalledWith('sb-show-errordisplay'); }); + +describe('STORY_INIT', () => { + it('supports path params', () => { + document.location = { + pathname: 'pathname', + search: '?path=/story/kind--story&bar=baz', + }; + + const render = jest.fn(); + const { clientApi } = start(render); + const store = clientApi._storyStore; + store.setSelection = jest.fn(); + store.emit(Events.STORY_INIT); + + store.emit(); + expect(store.setSelection).toHaveBeenCalledWith({ storyId: 'kind--story' }); + }); + + it('supports story kind/name params', () => { + document.location = { + pathname: 'pathname', + search: '?selectedKind=kind&selectedStory=story&bar=baz', + }; + + const render = jest.fn(); + const { clientApi } = start(render); + const store = clientApi._storyStore; + store.setSelection = jest.fn(); + + store.emit(Events.STORY_INIT); + expect(history.replaceState).toHaveBeenCalledWith({}, '', 'pathname?bar=baz&id=kind--story'); + expect(store.setSelection).toHaveBeenCalledWith({ storyId: 'kind--story' }); + }); +}); diff --git a/lib/core/src/client/preview/url.js b/lib/core/src/client/preview/url.js new file mode 100644 index 000000000000..12c1c18bb75b --- /dev/null +++ b/lib/core/src/client/preview/url.js @@ -0,0 +1,39 @@ +import { history, document } from 'global'; +import qs from 'qs'; +import { toId } from '@storybook/router/utils'; + +export function pathToId(path) { + const match = (path || '').match(/^\/story\/(.+)/); + if (!match) { + throw new Error(`Invalid path '${path}', must start with '/story/'`); + } + return match[1]; +} + +export const setPath = ({ storyId }) => { + const { path, selectedKind, selectedStory, ...rest } = qs.parse(document.location.search, { + ignoreQueryPrefix: true, + }); + const newPath = `${document.location.pathname}?${qs.stringify({ ...rest, id: storyId })}`; + history.replaceState({}, '', newPath); +}; + +export const getIdFromLegacyQuery = ({ path, selectedKind, selectedStory }) => + (path && pathToId(path)) || (selectedKind && selectedStory && toId(selectedKind, selectedStory)); + +export const parseQueryParameters = search => { + const { id } = qs.parse(search, { ignoreQueryPrefix: true }); + return id; +}; + +export const initializePath = () => { + const query = qs.parse(document.location.search, { ignoreQueryPrefix: true }); + let { id: storyId } = query; + if (!storyId) { + storyId = getIdFromLegacyQuery(query); + if (storyId) { + setPath({ storyId }); + } + } + return { storyId }; +}; diff --git a/lib/core/src/client/preview/url.test.js b/lib/core/src/client/preview/url.test.js new file mode 100644 index 000000000000..0a33955e4c71 --- /dev/null +++ b/lib/core/src/client/preview/url.test.js @@ -0,0 +1,79 @@ +import { history, document } from 'global'; +import { + pathToId, + setPath, + getIdFromLegacyQuery, + parseQueryParameters, + initializePath, +} from './url'; + +jest.mock('global', () => ({ + history: { replaceState: jest.fn() }, + document: { + location: { + pathname: 'pathname', + search: '', + }, + }, +})); + +describe('url', () => { + describe('pathToId', () => { + it('should parse valid ids', () => { + expect(pathToId('/story/story--id')).toEqual('story--id'); + }); + it('should error on invalid ids', () => { + [null, '', '/whatever/story/story--id'].forEach(path => { + expect(() => pathToId(path)).toThrow(/Invalid/); + }); + }); + }); + + describe('setPath', () => { + it('should navigate to storyId', () => { + setPath({ storyId: 'story--id' }); + expect(history.replaceState).toHaveBeenCalledWith({}, '', 'pathname?id=story--id'); + }); + it('should replace legacy parameters but preserve others', () => { + document.location.search = 'foo=bar&selectedStory=selStory&selectedKind=selKind'; + setPath({ storyId: 'story--id' }); + expect(history.replaceState).toHaveBeenCalledWith({}, '', 'pathname?foo=bar&id=story--id'); + }); + }); + + describe('getIdFromLegacyQuery', () => { + it('should parse story paths', () => { + expect(getIdFromLegacyQuery({ path: '/story/story--id' })).toBe('story--id'); + }); + it('should parse legacy queries', () => { + expect( + getIdFromLegacyQuery({ path: null, selectedKind: 'kind', selectedStory: 'story' }) + ).toBe('kind--story'); + }); + it('should not parse non-queries', () => { + expect(getIdFromLegacyQuery({})).toBeUndefined(); + }); + }); + + describe('parseQueryParameters', () => { + it('should parse id', () => { + expect(parseQueryParameters('?foo=bar&id=story--id')).toBe('story--id'); + }); + it('should not parse non-ids', () => { + expect(parseQueryParameters('')).toBeUndefined(); + }); + }); + + describe('initializePath', () => { + it('should handle id queries', () => { + document.location.search = '?id=story--id'; + expect(initializePath()).toEqual({ storyId: 'story--id' }); + expect(history.replaceState).not.toHaveBeenCalled(); + }); + it('should redirect legacy queries', () => { + document.location.search = '?selectedKind=kind&selectedStory=story'; + expect(initializePath()).toEqual({ storyId: 'kind--story' }); + expect(history.replaceState).toHaveBeenCalledWith({}, '', 'pathname?id=kind--story'); + }); + }); +}); From 1e81ea4fb4aafb5942e4a663e26f750c4defe65e Mon Sep 17 00:00:00 2001 From: Michael Shilman Date: Mon, 17 Jun 2019 18:07:42 +0800 Subject: [PATCH 2/2] Clean up comments --- lib/client-api/src/story_store.js | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/lib/client-api/src/story_store.js b/lib/client-api/src/story_store.js index b048c662ba31..9a7edd006358 100644 --- a/lib/client-api/src/story_store.js +++ b/lib/client-api/src/story_store.js @@ -6,10 +6,6 @@ import { stripIndents } from 'common-tags'; import Events from '@storybook/core-events'; import { logger } from '@storybook/client-logger'; -// import { toId } from '@storybook/router/utils'; - -// import pathToId from './pathToId'; -// import { getQueryParams } from './queryparams'; // TODO: these are copies from components/nav/lib // refactor to DRY @@ -46,19 +42,6 @@ export default class StoryStore extends EventEmitter { this._revision = 0; this._selection = {}; this._channel = params.channel; - - // this.on(Events.STORY_INIT, () => { - // let storyId = this.getIdOnPath(); - // if (!storyId) { - // const query = getQueryParams(); - // storyId = getIdFromLegacyQuery(query); - // if (storyId) { - // const { path, selectedKind, selectedStory, ...rest } = query; - // this.setPath(storyId, rest); - // } - // } - // this.setSelection(this.fromId(storyId)); - // }); } setChannel = channel => {