From 0a5466a32caade821dc2fe45439f16b560f4fe0a Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Wed, 27 Oct 2021 15:48:39 +1100 Subject: [PATCH] Always update initial args when loading a story And also update the current args with the delta. This means both when we change story (to an HMR-ed story) and when we HMR the current story. --- lib/preview-web/src/PreviewWeb.test.ts | 72 +++++++++ lib/preview-web/src/PreviewWeb.tsx | 2 - lib/store/src/ArgsStore.test.ts | 214 ++++++++++++------------- lib/store/src/ArgsStore.ts | 29 ++-- lib/store/src/StoryStore.ts | 2 +- 5 files changed, 191 insertions(+), 128 deletions(-) diff --git a/lib/preview-web/src/PreviewWeb.test.ts b/lib/preview-web/src/PreviewWeb.test.ts index e19dd2a39fb5..1f26b52ddd2a 100644 --- a/lib/preview-web/src/PreviewWeb.test.ts +++ b/lib/preview-web/src/PreviewWeb.test.ts @@ -2219,6 +2219,78 @@ describe('PreviewWeb', () => { }); }); + describe('when another (not current) story changes', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + afterEach(() => { + jest.useRealTimers(); + }); + const newComponentOneExports = merge({}, componentOneExports, { + a: { args: { bar: 'edited' }, argTypes: { bar: { type: { name: 'string' } } } }, + }); + const newImportFn = jest.fn(async (path) => { + return path === './src/ComponentOne.stories.js' + ? newComponentOneExports + : componentTwoExports; + }); + it('retains the same delta to the args', async () => { + // Start at Story A + document.location.search = '?id=component-one--a'; + const preview = await createAndRenderPreview(); + + // Change A's args + mockChannel.emit.mockClear(); + emitter.emit(Events.UPDATE_STORY_ARGS, { + storyId: 'component-one--a', + updatedArgs: { foo: 'updated' }, + }); + await waitForRender(); + + // Change to story B + mockChannel.emit.mockClear(); + emitter.emit(Events.SET_CURRENT_STORY, { + storyId: 'component-one--b', + viewMode: 'story', + }); + await waitForSetCurrentStory(); + await waitForRender(); + expect(preview.storyStore.args.get('component-one--a')).toEqual({ + foo: 'updated', + }); + + // Update story A's args via HMR + mockChannel.emit.mockClear(); + projectAnnotations.renderToDOM.mockClear(); + preview.onStoriesChanged({ importFn: newImportFn }); + await waitForRender(); + + // Change back to Story A + mockChannel.emit.mockClear(); + emitter.emit(Events.SET_CURRENT_STORY, { + storyId: 'component-one--a', + viewMode: 'story', + }); + await waitForSetCurrentStory(); + await waitForRender(); + expect(preview.storyStore.args.get('component-one--a')).toEqual({ + foo: 'updated', + bar: 'edited', + }); + + expect(projectAnnotations.renderToDOM).toHaveBeenCalledWith( + expect.objectContaining({ + forceRemount: true, + storyContext: expect.objectContaining({ + id: 'component-one--a', + args: { foo: 'updated', bar: 'edited' }, + }), + }), + undefined // this is coming from view.prepareForStory, not super important + ); + }); + }); + describe('if the story no longer exists', () => { const { a, ...componentOneExportsWithoutA } = componentOneExports; const newImportFn = jest.fn(async (path) => { diff --git a/lib/preview-web/src/PreviewWeb.tsx b/lib/preview-web/src/PreviewWeb.tsx index 6aba2d7c3e88..06aa9b6e359e 100644 --- a/lib/preview-web/src/PreviewWeb.tsx +++ b/lib/preview-web/src/PreviewWeb.tsx @@ -328,8 +328,6 @@ export class PreviewWeb { if (persistedArgs) { this.storyStore.args.updateFromPersisted(story, persistedArgs); - } else if (implementationChanged) { - this.storyStore.args.resetOnImplementationChange(story, this.previousStory); } // Don't re-render the story if nothing has changed to justify it diff --git a/lib/store/src/ArgsStore.test.ts b/lib/store/src/ArgsStore.test.ts index 19738277fc8c..31b7880d1fdb 100644 --- a/lib/store/src/ArgsStore.test.ts +++ b/lib/store/src/ArgsStore.test.ts @@ -9,14 +9,7 @@ describe('ArgsStore', () => { describe('setInitial / get', () => { it('returns in a straightforward way', () => { const store = new ArgsStore(); - store.setInitial('id', { foo: 'bar' }); - expect(store.get('id')).toEqual({ foo: 'bar' }); - }); - - it('does not allow re-setting', () => { - const store = new ArgsStore(); - store.setInitial('id', { foo: 'bar' }); - store.setInitial('id', { foo: 'baz' }); + store.setInitial({ id: 'id', initialArgs: { foo: 'bar' } } as any); expect(store.get('id')).toEqual({ foo: 'bar' }); }); @@ -24,13 +17,108 @@ describe('ArgsStore', () => { const store = new ArgsStore(); expect(() => store.get('id')).toThrow(/No args known/); }); + + describe('on second call for same story', () => { + describe('if initialArgs are unchanged', () => { + it('does nothing if the args are untouched', () => { + const store = new ArgsStore(); + + const previousStory = { + id: 'id', + initialArgs: { a: '1', b: '1' }, + argTypes: { a: stringType, b: stringType }, + } as any; + store.setInitial(previousStory); + + const story = { + id: 'id', + initialArgs: { a: '1', b: '1' }, + argTypes: { a: stringType, b: stringType }, + } as any; + + store.setInitial(story); + expect(store.get(story.id)).toEqual({ a: '1', b: '1' }); + }); + + it('retains any arg changes', () => { + const store = new ArgsStore(); + + const previousStory = { + id: 'id', + initialArgs: { a: '1', b: false, c: 'unchanged' }, + argTypes: { a: stringType, b: booleanType, c: stringType }, + } as any; + store.setInitial(previousStory); + + // NOTE: I'm not sure technically you should be allowed to set d here, but + // let's make sure we behave sensibly if you do + store.update('id', { a: 'update', b: true, d: 'update' }); + + const story = { + id: 'id', + initialArgs: { a: '1', b: false, c: 'unchanged' }, + argTypes: { a: stringType, b: booleanType, c: stringType }, + } as any; + + store.setInitial(story); + // In any case c is not retained. + expect(store.get(story.id)).toEqual({ a: 'update', b: true, c: 'unchanged' }); + }); + }); + + describe('when initialArgs change', () => { + it('replaces old args with new if the args are untouched', () => { + const store = new ArgsStore(); + + const previousStory = { + id: 'id', + initialArgs: { a: '1', b: '1' }, + argTypes: { a: stringType, b: stringType }, + } as any; + store.setInitial(previousStory); + + const story = { + id: 'id', + initialArgs: { a: '1', c: '1' }, + argTypes: { a: stringType, c: stringType }, + } as any; + + store.setInitial(story); + expect(store.get(story.id)).toEqual({ a: '1', c: '1' }); + }); + + it('applies the same delta if the args are changed', () => { + const store = new ArgsStore(); + + const previousStory = { + id: 'id', + initialArgs: { a: '1', b: '1' }, + argTypes: { a: stringType, b: stringType }, + } as any; + store.setInitial(previousStory); + + // NOTE: I'm not sure technically you should be allowed to set c here + store.update('id', { a: 'update', c: 'update' }); + + const story = { + id: 'id', + initialArgs: { a: '2', d: '2' }, + argTypes: { a: stringType, d: stringType }, + } as any; + + store.setInitial(story); + // In any case c is not retained. + expect(store.get(story.id)).toEqual({ a: 'update', d: '2' }); + }); + }); + }); }); describe('update', () => { it('overrides on a per-key basis', () => { const store = new ArgsStore(); - store.setInitial('id', {}); + store.setInitial({ id: 'id', initialArgs: {} } as any); store.update('id', { foo: 'bar' }); expect(store.get('id')).toEqual({ foo: 'bar' }); @@ -42,7 +130,7 @@ describe('ArgsStore', () => { it('does not merge objects', () => { const store = new ArgsStore(); - store.setInitial('id', {}); + store.setInitial({ id: 'id', initialArgs: {} } as any); store.update('id', { obj: { foo: 'bar' } }); expect(store.get('id')).toEqual({ obj: { foo: 'bar' } }); @@ -54,7 +142,7 @@ describe('ArgsStore', () => { it('does not set keys to undefined, it simply unsets them', () => { const store = new ArgsStore(); - store.setInitial('id', { foo: 'bar' }); + store.setInitial({ id: 'id', initialArgs: { foo: 'bar' } } as any); store.update('id', { foo: undefined }); expect('foo' in store.get('id')).toBe(false); @@ -65,7 +153,7 @@ describe('ArgsStore', () => { it('ensures the types of args are correct', () => { const store = new ArgsStore(); - store.setInitial('id', {}); + store.setInitial({ id: 'id', initialArgs: {} } as any); const story = { id: 'id', @@ -81,10 +169,7 @@ describe('ArgsStore', () => { it('merges objects and sparse arrays', () => { const store = new ArgsStore(); - store.setInitial('id', { - a: { foo: 'bar' }, - b: ['1', '2', '3'], - }); + store.setInitial({ id: 'id', initialArgs: { a: { foo: 'bar' }, b: ['1', '2', '3'] } } as any); const story = { id: 'id', @@ -110,7 +195,7 @@ describe('ArgsStore', () => { it('checks args are allowed options', () => { const store = new ArgsStore(); - store.setInitial('id', {}); + store.setInitial({ id: 'id', initialArgs: {} } as any); const story = { id: 'id', @@ -123,99 +208,4 @@ describe('ArgsStore', () => { expect(store.get('id')).toEqual({ a: 'a' }); }); }); - - describe('resetOnImplementationChange', () => { - describe('if initialArgs are unchanged', () => { - it('does nothing if the args are untouched', () => { - const store = new ArgsStore(); - - const previousStory = { - id: 'id', - initialArgs: { a: '1', b: '1' }, - argTypes: { a: stringType, b: stringType }, - } as any; - store.setInitial('id', previousStory.initialArgs); - - const story = { - id: 'id', - initialArgs: { a: '1', b: '1' }, - argTypes: { a: stringType, b: stringType }, - } as any; - - store.resetOnImplementationChange(story, previousStory); - expect(store.get(story.id)).toEqual({ a: '1', b: '1' }); - }); - - it('retains any arg changes', () => { - const store = new ArgsStore(); - - const previousStory = { - id: 'id', - initialArgs: { a: '1', b: false, c: 'unchanged' }, - argTypes: { a: stringType, b: booleanType, c: stringType }, - } as any; - store.setInitial('id', previousStory.initialArgs); - - // NOTE: I'm not sure technically you should be allowed to set d here, but - // let's make sure we behave sensibly if you do - store.update('id', { a: 'update', b: true, d: 'update' }); - - const story = { - id: 'id', - initialArgs: { a: '1', b: false, c: 'unchanged' }, - argTypes: { a: stringType, b: booleanType, c: stringType }, - } as any; - - store.resetOnImplementationChange(story, previousStory); - // In any case c is not retained. - expect(store.get(story.id)).toEqual({ a: 'update', b: true, c: 'unchanged' }); - }); - }); - - describe('when initialArgs change', () => { - it('replaces old args with new if the args are untouched', () => { - const store = new ArgsStore(); - - const previousStory = { - id: 'id', - initialArgs: { a: '1', b: '1' }, - argTypes: { a: stringType, b: stringType }, - } as any; - store.setInitial('id', previousStory.initialArgs); - - const story = { - id: 'id', - initialArgs: { a: '1', c: '1' }, - argTypes: { a: stringType, c: stringType }, - } as any; - - store.resetOnImplementationChange(story, previousStory); - expect(store.get(story.id)).toEqual({ a: '1', c: '1' }); - }); - - it('applies the same delta if the args are changed', () => { - const store = new ArgsStore(); - - const previousStory = { - id: 'id', - initialArgs: { a: '1', b: '1' }, - argTypes: { a: stringType, b: stringType }, - } as any; - store.setInitial('id', previousStory.initialArgs); - - // NOTE: I'm not sure technically you should be allowed to set c here - store.update('id', { a: 'update', c: 'update' }); - - const story = { - id: 'id', - initialArgs: { a: '2', d: '2' }, - argTypes: { a: stringType, d: stringType }, - } as any; - - store.resetOnImplementationChange(story, previousStory); - // In any case c is not retained. - expect(store.get(story.id)).toEqual({ a: 'update', d: '2' }); - }); - }); - }); }); diff --git a/lib/store/src/ArgsStore.ts b/lib/store/src/ArgsStore.ts index 95833bf4b844..97749e9777b9 100644 --- a/lib/store/src/ArgsStore.ts +++ b/lib/store/src/ArgsStore.ts @@ -1,4 +1,4 @@ -import { StoryId, Args } from '@storybook/csf'; +import { StoryId, Args, ArgTypes } from '@storybook/csf'; import { Story } from './types'; import { combineArgs, mapArgsToTypes, validateOptions, deepDiff, DEEPLY_EQUAL } from './args'; @@ -10,6 +10,8 @@ function deleteUndefined(obj: Record) { } export class ArgsStore { + initialArgsByStoryId: Record = {}; + argsByStoryId: Record = {}; get(storyId: StoryId) { @@ -20,9 +22,19 @@ export class ArgsStore { return this.argsByStoryId[storyId]; } - setInitial(storyId: StoryId, args: Args) { - if (!this.argsByStoryId[storyId]) { - this.argsByStoryId[storyId] = args; + setInitial(story: Story) { + if (!this.initialArgsByStoryId[story.id]) { + this.initialArgsByStoryId[story.id] = story.initialArgs; + this.argsByStoryId[story.id] = story.initialArgs; + } else { + // When we get a new version of a story (with new initialArgs), we re-apply the same diff + // that we had previously applied to the old version of the story + const delta = deepDiff(this.initialArgsByStoryId[story.id], this.argsByStoryId[story.id]); + this.initialArgsByStoryId[story.id] = story.initialArgs; + this.argsByStoryId[story.id] = story.initialArgs; + if (delta !== DEEPLY_EQUAL) { + this.updateFromDelta(story, delta); + } } } @@ -44,15 +56,6 @@ export class ArgsStore { return this.updateFromDelta(story, mappedPersisted); } - resetOnImplementationChange(story: Story, previousStory: Story) { - const delta = deepDiff(previousStory.initialArgs, this.get(story.id)); - - this.argsByStoryId[story.id] = story.initialArgs; - if (delta !== DEEPLY_EQUAL) { - this.updateFromDelta(story, delta); - } - } - update(storyId: StoryId, argsUpdate: Partial) { if (!(storyId in this.argsByStoryId)) { throw new Error(`No args known for ${storyId} -- has it been rendered yet?`); diff --git a/lib/store/src/StoryStore.ts b/lib/store/src/StoryStore.ts index d4235d31c172..2f190b3f16b1 100644 --- a/lib/store/src/StoryStore.ts +++ b/lib/store/src/StoryStore.ts @@ -263,7 +263,7 @@ export class StoryStore { componentAnnotations, this.projectAnnotations ); - this.args.setInitial(story.id, story.initialArgs); + this.args.setInitial(story); this.hooks[story.id] = this.hooks[story.id] || new HooksContext(); return story; }