From 98a99b5c2f4b9c8e6189c2d4d3dd1faeedea58ed Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Tue, 8 May 2018 21:07:19 +1000 Subject: [PATCH 1/7] Refactor transitional decorator from addon-notes This should make it easy to transitional other addons to a parameter-based style --- addons/notes/src/index.js | 34 ++++++++---------------- lib/addons/package.json | 3 ++- lib/addons/src/index.js | 1 + lib/addons/src/transitional-decorator.js | 34 ++++++++++++++++++++++++ 4 files changed, 48 insertions(+), 24 deletions(-) create mode 100644 lib/addons/src/transitional-decorator.js diff --git a/addons/notes/src/index.js b/addons/notes/src/index.js index aeb02d93e254..4133739ed133 100644 --- a/addons/notes/src/index.js +++ b/addons/notes/src/index.js @@ -1,4 +1,4 @@ -import addons from '@storybook/addons'; +import addons, { makeTransitionalDecorator } from '@storybook/addons'; import marked from 'marked'; function renderMarkdown(text, options) { @@ -6,13 +6,13 @@ function renderMarkdown(text, options) { return marked(text); } -const decorator = options => { - const channel = addons.getChannel(); - return (getStory, context) => { - const { - parameters: { notes }, - } = context; - const storyOptions = notes || options; +export const withNotes = makeTransitionalDecorator({ + name: 'withNotes', + parameterName: 'notes', + wrapper: (getStory, context, { options, parameters }) => { + const channel = addons.getChannel(); + + const storyOptions = parameters || options; if (storyOptions) { const { text, markdown, markdownOptions } = @@ -26,23 +26,11 @@ const decorator = options => { } return getStory(context); - }; -}; - -const hoc = options => story => context => decorator(options)(story, context); + }, +}); export const withMarkdownNotes = (text, options) => - hoc({ + withNotes({ markdown: text, markdownOptions: options, }); - -export const withNotes = (...args) => { - // Used without options as .addDecorator(withNotes) - if (typeof args[0] === 'function') { - return decorator()(...args); - } - - // Input are options, ala .add('name', withNotes('note')(() => )) - return hoc(args[0]); -}; diff --git a/lib/addons/package.json b/lib/addons/package.json index de1e720df0c7..9a9e53e0848c 100644 --- a/lib/addons/package.json +++ b/lib/addons/package.json @@ -21,6 +21,7 @@ }, "dependencies": { "@storybook/channels": "4.0.0-alpha.4", - "global": "^4.3.2" + "global": "^4.3.2", + "util-deprecate": "^1.0.2" } } diff --git a/lib/addons/src/index.js b/lib/addons/src/index.js index 161a58ed0067..e573c9000ece 100644 --- a/lib/addons/src/index.js +++ b/lib/addons/src/index.js @@ -2,6 +2,7 @@ import global from 'global'; export mockChannel from './storybook-channel-mock'; +export { makeTransitionalDecorator } from './transitional-decorator'; export class AddonStore { constructor() { diff --git a/lib/addons/src/transitional-decorator.js b/lib/addons/src/transitional-decorator.js new file mode 100644 index 000000000000..4ce96b48220b --- /dev/null +++ b/lib/addons/src/transitional-decorator.js @@ -0,0 +1,34 @@ +import deprecate from 'util-deprecate'; + +// Create a decorator that can be used both in the (deprecated) old "hoc" style: +// .add('story', decorator(options)(() => )); +// +// And in the new, "parameterized" style: +// .addDecorator(decorator) +// .add('story', () => , { name: { parameters } }); + +export const makeTransitionalDecorator = ({ name, parameterName, wrapper }) => { + const decorator = options => (getStory, context) => { + const parameters = context.parameters && context.parameters[parameterName]; + + return wrapper(getStory, context, { + options, + parameters, + }); + }; + + const hoc = deprecate( + options => story => context => decorator(options)(story, context), + `Passing stories directly into ${name}() is deprecated, instead use addDecorator(${name}) and pass options with the '${parameterName}' parameter` + ); + + return (...args) => { + // Used without options as .addDecorator(decorator) + if (typeof args[0] === 'function') { + return decorator()(...args); + } + + // Input are options, ala .add('name', decorator('note')(() => )) + return hoc(args[0]); + }; +}; From e7f7f32703449664b8b1b55a63bc6751a24f1c6f Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Mon, 14 May 2018 17:32:21 +1000 Subject: [PATCH 2/7] Ensure that `addDecorator(decorator(options))` is *NOT* marked deprecated --- lib/addons/src/transitional-decorator.js | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/lib/addons/src/transitional-decorator.js b/lib/addons/src/transitional-decorator.js index 4ce96b48220b..15e59cfa7169 100644 --- a/lib/addons/src/transitional-decorator.js +++ b/lib/addons/src/transitional-decorator.js @@ -17,18 +17,24 @@ export const makeTransitionalDecorator = ({ name, parameterName, wrapper }) => { }); }; - const hoc = deprecate( - options => story => context => decorator(options)(story, context), - `Passing stories directly into ${name}() is deprecated, instead use addDecorator(${name}) and pass options with the '${parameterName}' parameter` - ); - return (...args) => { // Used without options as .addDecorator(decorator) if (typeof args[0] === 'function') { return decorator()(...args); } - // Input are options, ala .add('name', decorator('note')(() => )) - return hoc(args[0]); + return (...innerArgs) => { + // Used as [.]addDecorator(decorator(options)) + if (typeof innerArgs.length > 1) { + return decorator(...args)(...innerArgs); + } + + // Used to wrap a story directly .add('story', decorator(options)(() => )) + // This is now deprecated: + return deprecate( + context => decorator(...args)(innerArgs[0], context), + `Passing stories directly into ${name}() is deprecated, instead use addDecorator(${name}) and pass options with the '${parameterName}' parameter` + ); + }; }; }; From 15c2ed84e75be69f235566162082fcd72baa1780 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Mon, 14 May 2018 17:36:20 +1000 Subject: [PATCH 3/7] Fix addon-notes test --- addons/notes/src/__tests__/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/addons/notes/src/__tests__/index.js b/addons/notes/src/__tests__/index.js index 611a8a2bb66e..995bb61e7cf2 100644 --- a/addons/notes/src/__tests__/index.js +++ b/addons/notes/src/__tests__/index.js @@ -1,7 +1,7 @@ import addons from '@storybook/addons'; import { withNotes } from '..'; -jest.mock('@storybook/addons'); +addons.getChannel = jest.fn(); describe('Storybook Addon Notes', () => { it('should inject text from `notes` parameter', () => { From b8f8acb398219b1e268fdeb62a4b057a4bb974ae Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Tue, 15 May 2018 13:12:46 +1000 Subject: [PATCH 4/7] Update CLI snapshots --- lib/cli/test/snapshots/marko/package.json | 2 +- lib/cli/test/snapshots/meteor/package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/cli/test/snapshots/marko/package.json b/lib/cli/test/snapshots/marko/package.json index c36c72561ebd..4ed1a5ddfd45 100644 --- a/lib/cli/test/snapshots/marko/package.json +++ b/lib/cli/test/snapshots/marko/package.json @@ -7,7 +7,7 @@ "marko": "^4.9.7" }, "devDependencies": { - "@storybook/marko": "4.0.0-alpha.4", + "@storybook/marko": "^4.0.0-alpha.4", "babel-core": "^6.26.3", "babel-runtime": "^6.26.0" }, diff --git a/lib/cli/test/snapshots/meteor/package.json b/lib/cli/test/snapshots/meteor/package.json index 21d45d896bd9..7d64298af7f1 100644 --- a/lib/cli/test/snapshots/meteor/package.json +++ b/lib/cli/test/snapshots/meteor/package.json @@ -15,7 +15,7 @@ }, "devDependencies": { "babel-core": "^6.26.3", - "babel-preset-env": "^1.6.1", + "babel-preset-env": "^1.7.0", "babel-preset-react": "^6.24.1", "babel-preset-stage-1": "^6.24.1", "babel-root-slash-import": "^1.1.0", From 199098ccb0e41b94f0884f65528b493fb76431f0 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Tue, 15 May 2018 13:18:20 +1000 Subject: [PATCH 5/7] Rename to `makeDecorator` --- addons/notes/src/index.js | 4 ++-- lib/addons/src/index.js | 2 +- .../src/{transitional-decorator.js => make-decorator.js} | 7 +++++-- 3 files changed, 8 insertions(+), 5 deletions(-) rename lib/addons/src/{transitional-decorator.js => make-decorator.js} (84%) diff --git a/addons/notes/src/index.js b/addons/notes/src/index.js index 4133739ed133..b8299b39d9d1 100644 --- a/addons/notes/src/index.js +++ b/addons/notes/src/index.js @@ -1,4 +1,4 @@ -import addons, { makeTransitionalDecorator } from '@storybook/addons'; +import addons, { makeDecorator } from '@storybook/addons'; import marked from 'marked'; function renderMarkdown(text, options) { @@ -6,7 +6,7 @@ function renderMarkdown(text, options) { return marked(text); } -export const withNotes = makeTransitionalDecorator({ +export const withNotes = makeDecorator({ name: 'withNotes', parameterName: 'notes', wrapper: (getStory, context, { options, parameters }) => { diff --git a/lib/addons/src/index.js b/lib/addons/src/index.js index e573c9000ece..699123781027 100644 --- a/lib/addons/src/index.js +++ b/lib/addons/src/index.js @@ -2,7 +2,7 @@ import global from 'global'; export mockChannel from './storybook-channel-mock'; -export { makeTransitionalDecorator } from './transitional-decorator'; +export { makeDecorator } from './make-decorator'; export class AddonStore { constructor() { diff --git a/lib/addons/src/transitional-decorator.js b/lib/addons/src/make-decorator.js similarity index 84% rename from lib/addons/src/transitional-decorator.js rename to lib/addons/src/make-decorator.js index 15e59cfa7169..a25ff539335f 100644 --- a/lib/addons/src/transitional-decorator.js +++ b/lib/addons/src/make-decorator.js @@ -6,8 +6,11 @@ import deprecate from 'util-deprecate'; // And in the new, "parameterized" style: // .addDecorator(decorator) // .add('story', () => , { name: { parameters } }); +// +// *And* in the older, but not deprecated, "pass options to decorator" style: +// .addDecorator(decorator(options)) -export const makeTransitionalDecorator = ({ name, parameterName, wrapper }) => { +export const makeDecorator = ({ name, parameterName, wrapper }) => { const decorator = options => (getStory, context) => { const parameters = context.parameters && context.parameters[parameterName]; @@ -25,7 +28,7 @@ export const makeTransitionalDecorator = ({ name, parameterName, wrapper }) => { return (...innerArgs) => { // Used as [.]addDecorator(decorator(options)) - if (typeof innerArgs.length > 1) { + if (innerArgs.length > 1) { return decorator(...args)(...innerArgs); } From 5d8624990ba4e865887fd0a425fe734fa6739754 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Tue, 15 May 2018 14:32:29 +1000 Subject: [PATCH 6/7] Added test for `makeDecorator` --- lib/addons/src/make-decorator.test.js | 76 +++++++++++++++++++++++ lib/core/src/client/preview/client_api.js | 2 +- 2 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 lib/addons/src/make-decorator.test.js diff --git a/lib/addons/src/make-decorator.test.js b/lib/addons/src/make-decorator.test.js new file mode 100644 index 000000000000..16dc428a0425 --- /dev/null +++ b/lib/addons/src/make-decorator.test.js @@ -0,0 +1,76 @@ +import deprecate from 'util-deprecate'; +import { makeDecorator } from './make-decorator'; +import { defaultDecorateStory } from '../../../lib/core/src/client/preview/client_api'; + +jest.mock('util-deprecate'); +let deprecatedFns = []; +deprecate.mockImplementation((fn, warning) => { + const deprecatedFn = jest.fn(fn); + deprecatedFns.push({ + deprecatedFn, + warning, + }); + return deprecatedFn; +}); + +describe('makeDecorator', () => { + it('returns a decorator that passes parameters on the parameters argument', () => { + const wrapper = jest.fn(); + const decorator = makeDecorator({ wrapper, name: 'test', parameterName: 'test' }); + const story = jest.fn(); + const decoratedStory = defaultDecorateStory(story, [decorator]); + + const context = { parameters: { test: 'test-val' } }; + decoratedStory(context); + + expect(wrapper).toHaveBeenCalledWith(expect.any(Function), context, { parameters: 'test-val' }); + }); + + it('passes options added at decoration time', () => { + const wrapper = jest.fn(); + const decorator = makeDecorator({ wrapper, name: 'test', parameterName: 'test' }); + const story = jest.fn(); + const options = 'test-val'; + const decoratedStory = defaultDecorateStory(story, [decorator(options)]); + + const context = {}; + decoratedStory(context); + + expect(wrapper).toHaveBeenCalledWith(expect.any(Function), context, { options: 'test-val' }); + }); + + it('passes both options *and* parameters at the same time', () => { + const wrapper = jest.fn(); + const decorator = makeDecorator({ wrapper, name: 'test', parameterName: 'test' }); + const story = jest.fn(); + const options = 'test-val'; + const decoratedStory = defaultDecorateStory(story, [decorator(options)]); + + const context = { parameters: { test: 'test-val' } }; + decoratedStory(context); + + expect(wrapper).toHaveBeenCalledWith(expect.any(Function), context, { + options: 'test-val', + parameters: 'test-val', + }); + }); + + it('passes options added at story time, but with a deprecation warning', () => { + deprecatedFns = []; + const wrapper = jest.fn(); + const decorator = makeDecorator({ wrapper, name: 'test', parameterName: 'test' }); + const options = 'test-val'; + const story = jest.fn(); + const decoratedStory = decorator(options)(story); + expect(deprecatedFns).toHaveLength(1); + expect(deprecatedFns[0].warning).toMatch('addDecorator(test)'); + + const context = {}; + decoratedStory(context); + + expect(wrapper).toHaveBeenCalledWith(expect.any(Function), context, { + options: 'test-val', + }); + expect(deprecatedFns[0].deprecatedFn).toHaveBeenCalled(); + }); +}); diff --git a/lib/core/src/client/preview/client_api.js b/lib/core/src/client/preview/client_api.js index c63cbfd4e0af..446767f6e5e4 100644 --- a/lib/core/src/client/preview/client_api.js +++ b/lib/core/src/client/preview/client_api.js @@ -4,7 +4,7 @@ import { logger } from '@storybook/client-logger'; import StoryStore from './story_store'; -const defaultDecorateStory = (getStory, decorators) => +export const defaultDecorateStory = (getStory, decorators) => decorators.reduce( (decorated, decorator) => context => decorator(() => decorated(context), context), getStory From a2e57cf7d6d895754ac6cdf976849762db0b9efb Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Tue, 15 May 2018 14:37:38 +1000 Subject: [PATCH 7/7] Added `skipIfNoParametersOrOptions` argument to `makeDecorator` --- addons/notes/src/__tests__/index.js | 12 +++++++++++ addons/notes/src/index.js | 15 +++++++------- lib/addons/src/make-decorator.js | 10 ++++++++- lib/addons/src/make-decorator.test.js | 30 +++++++++++++++++++++++++++ 4 files changed, 58 insertions(+), 9 deletions(-) diff --git a/addons/notes/src/__tests__/index.js b/addons/notes/src/__tests__/index.js index 995bb61e7cf2..836a8140cd82 100644 --- a/addons/notes/src/__tests__/index.js +++ b/addons/notes/src/__tests__/index.js @@ -16,6 +16,18 @@ describe('Storybook Addon Notes', () => { expect(getStory).toHaveBeenCalledWith(context); }); + it('should NOT inject text if no `notes` parameter', () => { + const channel = { emit: jest.fn() }; + addons.getChannel.mockReturnValue(channel); + + const getStory = jest.fn(); + const context = {}; + + withNotes(getStory, context); + expect(channel.emit).not.toHaveBeenCalled(); + expect(getStory).toHaveBeenCalledWith(context); + }); + it('should inject markdown from `notes.markdown` parameter', () => { const channel = { emit: jest.fn() }; addons.getChannel.mockReturnValue(channel); diff --git a/addons/notes/src/index.js b/addons/notes/src/index.js index b8299b39d9d1..1f983df6da1a 100644 --- a/addons/notes/src/index.js +++ b/addons/notes/src/index.js @@ -9,22 +9,21 @@ function renderMarkdown(text, options) { export const withNotes = makeDecorator({ name: 'withNotes', parameterName: 'notes', + skipIfNoParametersOrOptions: true, wrapper: (getStory, context, { options, parameters }) => { const channel = addons.getChannel(); const storyOptions = parameters || options; - if (storyOptions) { - const { text, markdown, markdownOptions } = - typeof storyOptions === 'string' ? { text: storyOptions } : storyOptions; + const { text, markdown, markdownOptions } = + typeof storyOptions === 'string' ? { text: storyOptions } : storyOptions; - if (!text && !markdown) { - throw new Error('You must set of one of `text` or `markdown` on the `notes` parameter'); - } - - channel.emit('storybook/notes/add_notes', text || renderMarkdown(markdown, markdownOptions)); + if (!text && !markdown) { + throw new Error('You must set of one of `text` or `markdown` on the `notes` parameter'); } + channel.emit('storybook/notes/add_notes', text || renderMarkdown(markdown, markdownOptions)); + return getStory(context); }, }); diff --git a/lib/addons/src/make-decorator.js b/lib/addons/src/make-decorator.js index a25ff539335f..09ec27b7299a 100644 --- a/lib/addons/src/make-decorator.js +++ b/lib/addons/src/make-decorator.js @@ -10,10 +10,18 @@ import deprecate from 'util-deprecate'; // *And* in the older, but not deprecated, "pass options to decorator" style: // .addDecorator(decorator(options)) -export const makeDecorator = ({ name, parameterName, wrapper }) => { +export const makeDecorator = ({ + name, + parameterName, + wrapper, + skipIfNoParametersOrOptions = false, +}) => { const decorator = options => (getStory, context) => { const parameters = context.parameters && context.parameters[parameterName]; + if (skipIfNoParametersOrOptions && !options && !parameters) { + return getStory(context); + } return wrapper(getStory, context, { options, parameters, diff --git a/lib/addons/src/make-decorator.test.js b/lib/addons/src/make-decorator.test.js index 16dc428a0425..be41fb9a9144 100644 --- a/lib/addons/src/make-decorator.test.js +++ b/lib/addons/src/make-decorator.test.js @@ -55,6 +55,36 @@ describe('makeDecorator', () => { }); }); + it('passes nothing if neither are supplied', () => { + const wrapper = jest.fn(); + const decorator = makeDecorator({ wrapper, name: 'test', parameterName: 'test' }); + const story = jest.fn(); + const decoratedStory = defaultDecorateStory(story, [decorator]); + + const context = {}; + decoratedStory(context); + + expect(wrapper).toHaveBeenCalledWith(expect.any(Function), context, {}); + }); + + it('calls the story directly if neither are supplied and skipIfNoParametersOrOptions is true', () => { + const wrapper = jest.fn(); + const decorator = makeDecorator({ + wrapper, + name: 'test', + parameterName: 'test', + skipIfNoParametersOrOptions: true, + }); + const story = jest.fn(); + const decoratedStory = defaultDecorateStory(story, [decorator]); + + const context = {}; + decoratedStory(context); + + expect(wrapper).not.toHaveBeenCalled(); + expect(story).toHaveBeenCalled(); + }); + it('passes options added at story time, but with a deprecation warning', () => { deprecatedFns = []; const wrapper = jest.fn();