Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor knobs - no longer include all runtimes #1832

Merged
merged 5 commits into from
Sep 11, 2017

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Sep 11, 2017

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.

@codecov
Copy link

codecov bot commented Sep 11, 2017

Codecov Report

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

Impacted file tree graph

@@               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.

@ndelangen ndelangen changed the title Ndelangen/restructure knobs Refactor knobs - no longer include all runtimes Sep 11, 2017
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@ndelangen ndelangen Sep 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will delete that file with 4.0.

Let's keep this as backwards compatible as possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK then the comment is wrong?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my bad indeed, good catch

// Knobs as dynamic variables.
stories.add('as dynamic variables', () => {
stories.add('as dynamic variables', withKnobsOptions(options)(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

require('./dist/register')

Can we do something analogous for frameworks?

@@ -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('')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

???

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK then the comment is wrong?

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we also need to export something?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't we also need to export something?

…knobs/{{framework}}

And other review comments
@Hypnosphi
Copy link
Member

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

@shilman
Copy link
Member

shilman commented Sep 11, 2017

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

@ndelangen ndelangen merged commit 18f5e34 into release/3.3 Sep 11, 2017
@ndelangen ndelangen deleted the ndelangen/restructure-knobs branch September 11, 2017 21:57
@tmeasday
Copy link
Member

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

@joscha
Copy link
Member

joscha commented Jan 16, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants