Refactor transitional decorator from addon-notes #3559

tmeasday · 2018-05-08T11:09:31Z

This should make it easy to transitional other addons to a parameter-based style

Hypnosphi · 2018-05-08T11:17:03Z

lib/addons/src/transitional-decorator.js

+
+  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`


Are we sure that we want to deprecate the old way? Honestly, it looks a lot more explicit to me

Oh, I thought we did?

I think the strongest argument in favour is if you want to pass parameters to more than one decorator for the same story:

.add('story', withNotes('foo')(backgrounds(['red', 'white'])(() => <Story />))) vs .add('story', () => <Story />, { notes: 'foo', backgrounds: ['red', 'white'] })

I was planning on changing all the documentation to not mention the old way at least, so deprecating made sense to me, but I'm open to whatever.

I think we could afford supporting both

@ndelangen WDYT

What is a benefit of having the old API supported?

You may want to supply the same parameters (e.g. backgrounds) to multiple stories

And even extract it into variable or module, like here:
https://github.com/storybooks/storybook/tree/master/addons/backgrounds#usage
("Of course it's easy to create a library module")

I mean my idea is that you would just add addDecorators(backgrounds) once in your .storybook/config.js once (like you need to add it to .storybook/addons.js), and then you would add the parameters to turn the addon on wherever you needed it.

That's perfect I think.

@tmeasday I just realised that those are two different usecases

// here withSomething is params => (storyFn, context) => output .addDecorator(withSomething(params)) // here it is params => storyFn => () => output .add('story', withSomething(params)(storyFn))

Actually addons that support both do some checks to figure out which usage is it.

Can we maybe just deprecate the latter but keep the former, because it's quite useful in cases like backgrounds?

Oh, now I see that the latter is the thing we're actually deprecating here, good

But the former would probably need to be supported when migrating other addons

Ok, I see what you are saying here. Let me take another pass at this.

Hypnosphi · 2018-05-13T21:42:00Z

Looks like tests are failing because of this:

https://github.com/storybooks/storybook/blob/98a99b5c2f4b9c8e6189c2d4d3dd1faeedea58ed/addons/notes/src/__tests__/index.js#L4

…ated

codecov · 2018-05-14T07:32:44Z

Codecov Report

Merging #3559 into master will decrease coverage by 0.08%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##           master    #3559      +/-   ##
==========================================
- Coverage   37.08%   36.99%   -0.09%     
==========================================
  Files         511      512       +1     
  Lines       10851    10857       +6     
  Branches      975     1002      +27     
==========================================
- Hits         4024     4017       -7     
- Misses       6227     6232       +5     
- Partials      600      608       +8

Impacted Files	Coverage Δ
lib/addons/src/index.js	`0% <0%> (ø)`	⬆️
lib/addons/src/make-decorator.js	`0% <0%> (ø)`
addons/notes/src/index.js	`84.61% <75%> (-5.39%)`	⬇️
addons/knobs/src/components/types/Object.js	`6.66% <0%> (ø)`	⬆️
addons/knobs/src/components/types/Text.js	`8.77% <0%> (ø)`	⬆️
lib/components/src/table/cell.js	`65.21% <0%> (ø)`	⬆️
lib/codemod/src/transforms/update-addon-info.js	`50% <0%> (ø)`	⬆️
addons/knobs/src/components/types/Button.js	`11.9% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/libs/is_mobile_device.js	`42.85% <0%> (ø)`	⬆️
...modules/ui/components/stories_panel/text_filter.js	`36.11% <0%> (ø)`	⬆️
... and 74 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38992be...a2e57cf. Read the comment docs.

tmeasday · 2018-05-14T07:50:50Z

Ok, did another pass. Let me know what you think @Hypnosphi. I suspect we may have to tweak it as we apply the pattern to the various addons.

Right now, you can use the decorator returned by makeTransitionalDecorator({ ..., wrapper }) in the following ways:

// 1. Pass parameters per-story, ala notes:

// We do this once
addDecorator(withNotes)

// Then, on every story, we do:
add('story', () => <Story />, { notes: 'Note' });

// In this case, wrapper is called with:
wrapper(story, context, { parameters: 'Note' });

// If a story does *not* set the parameters, the wrapper *IS STILL CALLED* but without options:
// It is the responsibility of the wrapper to ignore this story [A]:
wrapper(story, context, {});

// 2. Adding global parameters at "decoration-time", ala backgrounds:

// This is it:
addDecorator(withBackgrounds(['blue', 'red']));

// In this case, wrapper is called (per-story) with the options set [B]:
wrapper(story, context, { options: ['blue', 'red'] });

// 2a. [equivalent, C] Also, you can achieve the above with `addDecorator` + `addParameters`:
addDecorator(withBackgrounds)
addParameters({ backgrounds: ['blue', 'red'] });

// In this case, wrapper is called with parameters set [B]:
wrapper(story, context, { parameters: ['blue', 'red'] }); 

// 3. Per-story via wrapping directly (notes, deprecated):

.add('story', withNotes('Note')(() => <Story />));

// This will call the wrapper with options set:
// *BUT* will trigger deprecation warnings.
wrapper(story, context, { options: 'Note' });

Notes from above:
[A]: We could take responsibility to ignore stories instead? I'm guessing some decorators like addon-info might want this, and require users to manually option-out ({info: { skip: true } } perhaps?).

[B] We could reconcile the two here? (i.e. make it parameters in both cases?). Potentially the addon cares about the difference though?

[C] Although 2a. is a bit more verbose it is also more powerful (e.g. you can set background parameters for the whole app, for a specific chapter, and for a specific story, and the parameters mechanism handles reconciliation automatically).

Hypnosphi · 2018-05-14T11:35:49Z

lib/addons/src/transitional-decorator.js

+//   .addDecorator(decorator)
+//   .add('story', () => <Story />, { name: { parameters } });
+
+export const makeTransitionalDecorator = ({ name, parameterName, wrapper }) => {


Should we rename it to something like makePolymorphicDecorator?

Sure! Or just makeDecorator()?

just makeDecorator sounds good

I like the makeDecorator name too

Hypnosphi · 2018-05-14T11:39:12Z

[A] In viewport addon, parameters are optional, so we should allow addons to handle stories without parameters

[B] I'm Ok with both

[C] I see. I still think that it wouldn't be too hard to support both versions

Hypnosphi · 2018-05-14T11:41:56Z

lib/addons/src/transitional-decorator.js

+
+    return (...innerArgs) => {
+      // Used as [.]addDecorator(decorator(options))
+      if (typeof innerArgs.length > 1) {


I need to add a test for this modality, it just doesn't really make sense for notes. I'll do another PR for backgrounds and we'll definitely have one.

tmeasday · 2018-05-14T12:19:17Z

[A] In viewport addon, parameters are optional, so we should allow addons to handle stories without parameters

Ok cool. We could also add an argument to the creator skipIfNoParameters: true or the like?

[B] I'm Ok with both

👍

[C] I see. I still think that it wouldn't be too hard to support both versions

Oh, yeah, that is the plan. Sorry if I was confusing. I was saying we support both, even though there is some overlap.

…ransitional-decorator

tmeasday · 2018-05-15T04:11:40Z

Not sure what's happening with the UI change to the angular CLI.

I'll add a unit test to the makeDecorator function

tmeasday · 2018-05-15T06:37:59Z

@Hypnosphi do you want to re-review this, I added an extra API and some tests

tmeasday · 2018-05-15T06:38:12Z

(Or just merge away)

Hypnosphi · 2018-05-17T23:29:02Z

Released as 4.0.0-alpha.7

Refactor transitional decorator from addon-notes

98a99b5

This should make it easy to transitional other addons to a parameter-based style

tmeasday added maintenance User-facing maintenance tasks addon: actions labels May 8, 2018

tmeasday requested review from ndelangen and Hypnosphi May 8, 2018 11:09

tmeasday requested review from alexandrebodin and theinterned as code owners May 8, 2018 11:09

Hypnosphi reviewed May 8, 2018

View reviewed changes

Hypnosphi approved these changes May 12, 2018

View reviewed changes

Ensure that addDecorator(decorator(options)) is *NOT* marked deprec…

e7f7f32

…ated

Fix addon-notes test

15c2ed8

Hypnosphi reviewed May 14, 2018

View reviewed changes

tmeasday added 2 commits May 15, 2018 13:12

Update CLI snapshots

b8f8acb

Rename to makeDecorator

199098c

tmeasday requested review from shilman and stijnkoopal as code owners May 15, 2018 03:19

Merge remote-tracking branch 'origin/master' into tmeasday/refactor-t…

de1c65c

…ransitional-decorator

tmeasday added 2 commits May 15, 2018 14:32

Added test for makeDecorator

5d86249

Added skipIfNoParametersOrOptions argument to makeDecorator

a2e57cf

tmeasday requested a review from alterx as a code owner May 15, 2018 04:37

Hypnosphi approved these changes May 15, 2018

View reviewed changes

Hypnosphi merged commit f2019e3 into master May 15, 2018

Hypnosphi deleted the tmeasday/refactor-transitional-decorator branch May 15, 2018 22:10

Hypnosphi mentioned this pull request May 16, 2018

Is is possible to override the values of global addons #3595

Closed

mshaaban088 mentioned this pull request May 20, 2018

addon-viewport: Use the new parameterized way of decorators #3610

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor transitional decorator from addon-notes #3559

Refactor transitional decorator from addon-notes #3559

tmeasday commented May 8, 2018

Hypnosphi May 8, 2018

tmeasday May 8, 2018

Hypnosphi May 8, 2018

igor-dv May 8, 2018

Hypnosphi May 8, 2018

ndelangen May 11, 2018

Hypnosphi May 12, 2018

Hypnosphi May 12, 2018

Hypnosphi May 13, 2018

tmeasday May 14, 2018

Hypnosphi commented May 13, 2018

codecov bot commented May 14, 2018 •

edited

Loading

tmeasday commented May 14, 2018

Hypnosphi May 14, 2018

tmeasday May 14, 2018

Hypnosphi May 14, 2018

ndelangen May 14, 2018

Hypnosphi commented May 14, 2018 •

edited

Loading

Hypnosphi May 14, 2018

tmeasday May 14, 2018

tmeasday May 15, 2018

tmeasday commented May 14, 2018

tmeasday commented May 15, 2018

tmeasday commented May 15, 2018

tmeasday commented May 15, 2018

Hypnosphi commented May 17, 2018

Refactor transitional decorator from addon-notes #3559

Refactor transitional decorator from addon-notes #3559

Conversation

tmeasday commented May 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypnosphi commented May 13, 2018

codecov bot commented May 14, 2018 • edited Loading

Codecov Report

tmeasday commented May 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypnosphi commented May 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmeasday commented May 14, 2018

tmeasday commented May 15, 2018

tmeasday commented May 15, 2018

tmeasday commented May 15, 2018

Hypnosphi commented May 17, 2018

codecov bot commented May 14, 2018 •

edited

Loading

Hypnosphi commented May 14, 2018 •

edited

Loading