Proposal for withX api change

[ndelangen]

Issue: #1383

One of the blockers for 3.2 release is the issue of addonKnobs and addonNotes.
We want to change the fundamental nature of how the addons work, but in a non-disruptive gradual way. We need to, to support more addons for more libraries/frameworks.

Previously addonX was introduced, but after some discussion we agreed this was not the right way forward. We'd like to continue with the phrasing of withX. However cannot simply detect which api the user is expecting.

What I did

I introduced a with export for 2 addons.
If you like this idea, we can work towards making all addons have this.

How to test

run install, bootstrap, test
run vue example and view knobs and notes
run react example and view knobs and notes

[tmeasday]

However cannot simply detect which api the user is expecting

Is this true? Wouldn't withKnobs get called with one of the following three signatures:

1. withKnobs(storyFn) -- wrapper decorator, used like withKnobs(() => <story>)
2. withKnobs(object) -- wrapper decorator with options, like withKnobs(options)(() => <story/>)
3. withKnobs(storyFn, context) -- "real" decorator, like addDecorator(withKnobs)?

I'm not 100% convinced polymorphising the API is a great idea, but I'm also not convinced withKnobsV2 is a great API either :/.

Also- don't you have a plan around a totally different API for knobs? Should we just wait for that?

[shilman]

Echoing @tmeasday 's comment, I think withKnobsV2 could be a crime against humanity, and I want to go on record to say don't merge until we've had a chance to discuss. Don't have a counter-proposal yet, but going to try to chat with @tmeasday matter later tonight, since I know we want to get a release out soon. 👍

[tmeasday]

A third idea here:

Perhaps we can export a single symbol (withKnobs) but hang different properties off it to apply in different contexts. Like withKnobs.decorator = X etc. This would be more explicit but equally user-friendly.

(cf webpack loader.pitch, et al)

[This would require core storybook changes however].

[ndelangen]

Like withKnobs.decorator = X etc.

I thought of this as well.. But we can't:

[This would require core storybook changes however].

The thing we don't want to do at this stage

[ndelangen]

I'll transform this into a new proposal this evening, hopefully we can agree it 😃

[codecov]

Codecov Report

Merging #1524 into release/3.2 will decrease coverage by 0.05%.
The diff coverage is 0%.

@@               Coverage Diff               @@
##           release/3.2    #1524      +/-   ##
===============================================
- Coverage        20.54%   20.49%   -0.06%     
===============================================
  Files              241      241              
  Lines             5227     5222       -5     
  Branches           638      645       +7     
===============================================
- Hits              1074     1070       -4     
+ Misses            3672     3652      -20     
- Partials           481      500      +19

Impacted Files	Coverage Δ
addons/knobs/src/index.js	0% <0%> (ø)	⬆️
addons/notes/src/index.js	0% <0%> (ø)	⬆️
app/vue/src/client/preview/client_api.js	86.84% <0%> (-0.66%)	⬇️
app/react/src/client/preview/client_api.js	88.09% <0%> (-0.55%)	⬇️
lib/ui/src/libs/key_events.js	23.25% <0%> (ø)	⬆️
addons/storyshots/src/require_context.js	0% <0%> (ø)	⬆️
lib/ui/src/modules/ui/configs/handle_keyevents.js	33.33% <0%> (ø)	⬆️
addons/knobs/src/components/PropForm.js	8.51% <0%> (ø)	⬆️
addons/knobs/src/components/types/Boolean.js	11.62% <0%> (ø)	⬆️
lib/ui/src/modules/api/configs/init_api.js	40.47% <0%> (ø)	⬆️
... and 18 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 21e4081...93414cd. Read the comment docs.

[ndelangen]

So please please review this, I hope you like this proposal better @storybooks/team

[ndelangen]

@danielduan maybe you'd like to pitch in here?

[danielduan]

I'm not sure if I am in favor of this one either. This seems to solve our architecture issue to a certain extent but it doesn't improve the user experience.

On the usability side, it's very tedious that we have all these withX(withY()) on the individual stories that still have to be explicitly defined every time. I wonder if there is a way to define some sort of inheritance structure as well.

Also, it doesn't seem like withSmartKnobs is included in our example app, maybe that should be something as well?

[ndelangen]

If you're not a fan of: withX(withY()) I don't blame you, it's not my favorite either.
There are solutions for it like compose(withX(), withY(), renderFn), and sometime in the future we'd want to improve this even more. Something to the kin of add('name', () => {}, [withX(), withY()])

But I think this is something that's separate from/larger then this PR

I wonder if we're looking for a perfect solution and backing ourselves in a corner: Can't change the existing (inconsistent) api, which is similarly names to what we current have but want to change in functionality.

[shilman]

@ndelangen closing this because it got superceded by #1527