Refactor knobs - no longer include all runtimes #1832

ndelangen · 2017-09-11T12:39:39Z

Issue: -addon knobs is getting some bad issues with the current way of switching platform-

Currently knobs is referencing all platform runtimes in index.js... Switching happens based on a global. Both are fairly bad practices. This PR addresses both.

It's fully backwards compatible.

But it does introduce a new way for consuming the knobs module, using deep file imports for each specific platform.

What I did

I've changed the way each platform would import addon-knobs.
I've detailed what changed in the readme, and have added a deprecation warning for the old way.
The old way still works.

How to test

Tests should pass
Documentation should be correct
Running all examples and playing around with knobs should work as expected.

I tested cra-kitchen-sink, vue-kitchen-sink, react-native-vanilla.

When using knobs on a storybook for react, it should not include anything other then react code.

codecov · 2017-09-11T12:50:38Z

Codecov Report

Merging #1832 into release/3.3 will decrease coverage by 0.13%.
The diff coverage is 2.22%.

@@               Coverage Diff               @@
##           release/3.3    #1832      +/-   ##
===============================================
- Coverage        22.45%   22.31%   -0.14%     
===============================================
  Files              321      324       +3     
  Lines             6280     6322      +42     
  Branches           796      787       -9     
===============================================
+ Hits              1410     1411       +1     
- Misses            4268     4322      +54     
+ Partials           602      589      -13

Impacted Files	Coverage Δ
addons/knobs/src/index.js	`0% <ø> (ø)`	⬆️
addons/knobs/src/vue/index.js	`22.72% <0%> (-8.53%)`	⬇️
addons/knobs/src/react/index.js	`40% <0%> (-60%)`	⬇️
addons/knobs/react.js	`0% <0%> (ø)`
addons/knobs/vue.js	`0% <0%> (ø)`
addons/knobs/src/base.js	`2.5% <3.22%> (ø)`
app/react-native/src/bin/storybook-build.js	`0% <0%> (ø)`	⬆️
addons/knobs/src/components/PropField.js	`10.86% <0%> (ø)`	⬆️
addons/centered/src/vue.js	`0% <0%> (ø)`	⬆️
addons/info/src/components/markdown/htags.js	`30% <0%> (ø)`	⬆️
... and 41 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 65d1dda...8c23357. Read the comment docs.

shilman

Great! Looks generally reasonable to me. I'm not exactly sure which of my comments are change requests and which are just discussion/suggestions, but we can figure it out.

shilman · 2017-09-11T19:37:47Z

addons/knobs/src/vue/index.js

@@ -35,3 +41,23 @@ export const vueHandler = (channel, knobStore) => getStory => context => ({
    knobStore.unsubscribe(this.setPaneKnobs);
  },
 });
+
+// "Higher order component" / wrapper style API
+// In 3.3, this will become `withKnobs`, once our decorator API supports it.


This is 3.3? Should we handle this API migration now?

We will delete that file with 4.0.

Let's keep this as backwards compatible as possible.

OK then the comment is wrong?

Also, I think the solution @tmeasday and I have in mind is backwards-compatible.

shilman · 2017-09-11T19:38:57Z

addons/knobs/README.md

@@ -37,7 +40,7 @@ Now, write your stories with knobs.

 ```js
 import { storiesOf } from '@storybook/react';
-import { withKnobs, text, boolean, number } from '@storybook/addon-knobs';
+import { addonKnobs, text, boolean, number } from '@storybook/addon-knobs/dist/react';


why did addonKnobs come back? is this a bad merge?

my bad indeed, good catch

shilman · 2017-09-11T19:40:18Z

addons/knobs/README.md

 // Knobs as dynamic variables.
-stories.add('as dynamic variables', () => {
+stories.add('as dynamic variables', withKnobsOptions(options)(() => {


Can we make this withKnobs(options)? To make it consistent with withNotes / withInfo?
OR can we make this wrapperKnobs(options) if we are not ready to migrate the addon API?

shilman · 2017-09-11T19:41:55Z

addons/knobs/src/index.js

-}
+deprecate(
+  () => {},
+  'Using @storybook/knobs directly is discouraged, please use @storybook/knobs/dist/{{framework}}'


Addons currently adopt the following es5 pattern for registration /register.js:

require('./dist/register')

Can we do something analogous for frameworks?

shilman · 2017-09-11T20:26:14Z

addons/knobs/README.md

@@ -237,6 +254,10 @@ If you feel like this addon is not performing well enough there is an option to
 Usage:

 ```js
+import { storiesOf } from '@storybook/react';
+
+const stories = storiesOf('')


shilman · 2017-09-11T20:27:57Z

addons/knobs/src/vue/index.js

@@ -35,3 +41,23 @@ export const vueHandler = (channel, knobStore) => getStory => context => ({
    knobStore.unsubscribe(this.setPaneKnobs);
  },
 });
+
+// "Higher order component" / wrapper style API
+// In 3.3, this will become `withKnobs`, once our decorator API supports it.


OK then the comment is wrong?

shilman · 2017-09-11T20:28:35Z

addons/knobs/src/vue/index.js

@@ -35,3 +41,23 @@ export const vueHandler = (channel, knobStore) => getStory => context => ({
    knobStore.unsubscribe(this.setPaneKnobs);
  },
 });
+
+// "Higher order component" / wrapper style API
+// In 3.3, this will become `withKnobs`, once our decorator API supports it.


Also, I think the solution @tmeasday and I have in mind is backwards-compatible.

shilman · 2017-09-11T20:29:09Z

addons/knobs/vue.js

@@ -0,0 +1 @@
+require('./dist/vue');


don't we also need to export something?

shilman · 2017-09-11T20:29:29Z

addons/knobs/react.js

@@ -0,0 +1 @@
+require('./dist/react');


don't we also need to export something?

…knobs/{{framework}} And other review comments

Hypnosphi · 2017-09-11T20:33:37Z

I agree with concerns by @shilman, otherwise looks good to me

…storybook/addon-knobs directly

shilman · 2017-09-11T21:10:38Z

@tmeasday mind taking a look at this before we merge?

tmeasday · 2017-09-11T23:05:12Z

Just looked at this, generally speaking looks like a very reasonable change. @shilman what was the final API change for knobs?

joscha · 2018-01-16T04:30:57Z

This needs a TS type change - I opened a PR: DefinitelyTyped/DefinitelyTyped#22939

ndelangen added 3 commits September 11, 2017 13:20

REFACTOR addon-knobs so it will not include runtimes of other platforms

003912a

When using knobs on a storybook for react, it should not include anything other then react code.

CHANGE examples to use new refactored addon-knobs

f5b0596

UPDATE the documentation for addon-knobs

9f147b5

ndelangen added addon: knobs api: addons labels Sep 11, 2017

ndelangen self-assigned this Sep 11, 2017

ndelangen requested a review from a team September 11, 2017 12:39

ndelangen changed the title ~~Ndelangen/restructure knobs~~ Refactor knobs - no longer include all runtimes Sep 11, 2017

shilman requested changes Sep 11, 2017

View reviewed changes

CHANGE @storybook/addon-knobs/dist/{{framework}} to @storybook/addon-…

5e74ef8

…knobs/{{framework}} And other review comments

ndelangen force-pushed the ndelangen/restructure-knobs branch from c87557d to 5e74ef8 Compare September 11, 2017 20:31

CORRECT readme for knobs && change comment on deprecating requiring @…

8c23357

…storybook/addon-knobs directly

ndelangen force-pushed the ndelangen/restructure-knobs branch from ca11103 to 8c23357 Compare September 11, 2017 20:55

shilman approved these changes Sep 11, 2017

View reviewed changes

Hypnosphi approved these changes Sep 11, 2017

View reviewed changes

ndelangen merged commit 18f5e34 into release/3.3 Sep 11, 2017

ndelangen deleted the ndelangen/restructure-knobs branch September 11, 2017 21:57

joscha mentioned this pull request Jan 16, 2018

@storybook/addon-knobs: add framework-specific entry points DefinitelyTyped/DefinitelyTyped#22939

Merged

8 tasks

Hypnosphi mentioned this pull request Apr 25, 2018

feat: add Vue support to @addons/background #3488

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor knobs - no longer include all runtimes #1832

Refactor knobs - no longer include all runtimes #1832

ndelangen commented Sep 11, 2017 •

edited

Loading

codecov bot commented Sep 11, 2017 •

edited

Loading

shilman left a comment

shilman Sep 11, 2017

ndelangen Sep 11, 2017 •

edited

Loading

shilman Sep 11, 2017

shilman Sep 11, 2017

shilman Sep 11, 2017

ndelangen Sep 11, 2017

shilman Sep 11, 2017

shilman Sep 11, 2017

shilman Sep 11, 2017

shilman Sep 11, 2017

shilman Sep 11, 2017

shilman Sep 11, 2017

shilman Sep 11, 2017

Hypnosphi commented Sep 11, 2017

shilman commented Sep 11, 2017

tmeasday commented Sep 11, 2017

joscha commented Jan 16, 2018

		@@ -0,0 +1 @@
		require('./dist/vue');

		@@ -0,0 +1 @@
		require('./dist/react');

Refactor knobs - no longer include all runtimes #1832

Refactor knobs - no longer include all runtimes #1832

Conversation

ndelangen commented Sep 11, 2017 • edited Loading

What I did

How to test

codecov bot commented Sep 11, 2017 • edited Loading

Codecov Report

shilman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndelangen Sep 11, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypnosphi commented Sep 11, 2017

shilman commented Sep 11, 2017

tmeasday commented Sep 11, 2017

joscha commented Jan 16, 2018

ndelangen commented Sep 11, 2017 •

edited

Loading

codecov bot commented Sep 11, 2017 •

edited

Loading

ndelangen Sep 11, 2017 •

edited

Loading