Import demo components from @storybook/react #1303

shilman · 2017-06-17T12:53:41Z

What I did

Import demo components from @storybook/react instead of @storybook/components

We do this so we can have a uniform way to do this across frameworks
(e.g. RN, vue)

How to test

To make sure the deps work:

yarn bootstrap
cd examples/cra-kitchen-sink
yarn storybook

Does not test getstorybook, but I think we can assume that will work (but please review)

We do this so we can have a uniform way to do this across frameworks (e.g. RN, vue)

codecov · 2017-06-17T13:05:10Z

Codecov Report

Merging #1303 into master will decrease coverage by <.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1303      +/-   ##
==========================================
- Coverage   13.76%   13.76%   -0.01%     
==========================================
  Files         201      202       +1     
  Lines        4620     4621       +1     
  Branches      507      574      +67     
==========================================
  Hits          636      636              
+ Misses       3550     3493      -57     
- Partials      434      492      +58

Impacted Files	Coverage Δ
app/react/src/demo/Button.js	`0% <ø> (ø)`
app/react/src/demo/Welcome.js	`0% <ø> (ø)`
...ts/stories/required_with_context/Button.stories.js	`0% <0%> (ø)`	⬆️
...s/stories/required_with_context/Welcome.stories.js	`0% <0%> (ø)`	⬆️
app/react/demo.js	`0% <0%> (ø)`
...dons/storyshots/stories/directly_required/index.js	`0% <0%> (ø)`	⬆️
app/react/src/client/preview/init.js	`0% <0%> (ø)`	⬆️
lib/ui/src/libs/key_events.js	`23.25% <0%> (ø)`	⬆️
app/react-native/src/preview/story_kind.js	`0% <0%> (ø)`	⬆️
app/react-native/src/bin/storybook-build.js	`0% <0%> (ø)`	⬆️
... and 27 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 3a27e21...c61e045. Read the comment docs.

usulpro

works fine for me!

alexandrebodin

it works

usulpro · 2017-06-17T14:12:13Z

still WIP? need to remove \lib\components\src\demo

shilman · 2017-06-17T14:21:05Z

Thanks @usulpro . I forgot to remove them earlier

shilman · 2017-06-17T14:38:16Z

@ndelangen @usulpro @alexandrebodin a test is failing but I don't have time to figure it out 😭

 FAIL  examples/test-cra/src/storyshots.test.js
 Test suite failed to run

Your test suite must contain at least one test.

WIP

alexandrebodin

We could create a demo.js in the root folder to reexport the components from dist ?

I don't like that user can access filles like this :(

Juste a preference no big deal for me ;)

shilman · 2017-06-17T19:07:49Z

Need to be offline for next four days. Would ❤️ if somebody could get this merged and shipped. @ndelangen @usulpro ?

usulpro · 2017-06-17T22:58:34Z

addons/storyshots/stories/directly_required/index.js

@@ -3,7 +3,7 @@ import React from 'react';
 import { storiesOf } from '@storybook/react'; // eslint-disable-line
 import { action } from '@storybook/addon-actions'; // eslint-disable-line

-import Button from '@storybook/components/dist/demo/Button';
+import Button from '@storybook/react/dist/demo/Button';


investigated that test fails after this change
not sure what the reason, possible someone who more familiar with storyshots could help?

FYI these particular files are not tested by CI

it fails at examples/test-cra/src/storyshots.test.js but after this change.
#2833 passed
#2836 failed

but possible it's more CI related issue then this PR?

I'm not sure why it sometimes passes to be honest, I would have thought it would consistently fail due to: #1303 (comment)

ndelangen · 2017-06-19T09:50:36Z

Looks good to me, what's failing the build?

tmeasday · 2017-06-21T11:38:25Z

Cf. #1286 -- not sure this test passes on any branch with npm5.

In this case the issue is that storyshots and the app have different copies of @storybook/react because it is loaded over a symlink. So storyshots finds no stories.

We should make storyshots show a better error if it finds no stories -- #1330

tmeasday · 2017-06-21T11:35:58Z

app/react-native/package.json

@@ -28,7 +28,7 @@
    "@storybook/addon-links": "^3.1.2",
    "@storybook/addons": "^3.1.2",
    "@storybook/channel-websocket": "^3.1.2",
-    "@storybook/components": "^3.0.0",
+    "@storybook/react": "^3.0.0",


Not sure we should be doing this

Does RN use these components? AFAIK not (if so it should be changed in this PR).

tmeasday · 2017-06-21T11:39:12Z

addons/storyshots/stories/directly_required/index.js

@@ -3,7 +3,7 @@ import React from 'react';
 import { storiesOf } from '@storybook/react'; // eslint-disable-line
 import { action } from '@storybook/addon-actions'; // eslint-disable-line

-import Button from '@storybook/components/dist/demo/Button';
+import Button from '@storybook/react/dist/demo/Button';


FYI these particular files are not tested by CI

shilman · 2017-06-21T13:56:52Z

Update

Currently the build is broken because test-cra requires @storybook/addon-storyshots AND @storybook/react. At the same time @storybook/addon-storyshots requires @storybook/react.

When we run the test-cra tests, two versions of @storybook/react are getting loaded, and this leads to a test error “Your test suite must contain at least one test.”

If we remove @storybook/react from @storybook/addon-storyshots's package.json then the tests pass again.

However, we are using demo components from @storybook/react in @storybook/addon-storyshots, so this "fix" is a filthy hack.

This is a hack. Currently the build is broken because `test-cra` requires `@storybook/addon-storyshots` AND `@storybook/react`, but `@storybook/addon-storyshots` requires `@storybook/react`. Before this change, two versions of `@storybook/react` are getting loaded, and this leads to a test error “Your test suite must contain at least one test.” After this “fix” the error goes away. However, `@storybook/addon-storyshots` should have a dev- dependency on `@storybook/react`, because it uses it for its examples. But it only has a peer-dep for now.

shilman · 2017-06-21T15:01:34Z

Update again

I have fixed the build with an unfortunate hack. @storybook/addon-storyshots now only takes @storybook/react as a peer-dep, rather than as a dev dep.

This sucks, and is not technically correct, but it unbreaks the build. I am thinking about alternative approaches to this whole mess.

@tmeasday

Per discussion with @tmeasday 😉

As suggested by npm/npm#15900. This is a workaround until we get a better resolution of #1286

Use a pack -> install technique to recreate local packages

shilman · 2017-06-22T05:10:56Z

@alexandrebodin i missed your suggestion above about creating a demo.js file. it's a great suggestion and i've incorporated it into this PR

shilman · 2017-06-22T05:23:53Z

Triumph!

@tmeasday 's PR uses tarballs for test-cra. This is a better solution than my hack above, so we merged his solution in and backed my hack out. Merging.

In the words of @tmeasday :

we ended up not removing test-cra, but instead using a npm pack-based strategy to reproduce the old file:// local package behaviour so test-cra retains the "flat" node_modules structure without symlinks. In particular, this works around issues with Jest misbehaving with symlinks, and allows us to test storyshots

Export demo components from @storybook/react

cd297c3

We do this so we can have a uniform way to do this across frameworks (e.g. RN, vue)

shilman added the in progress label Jun 17, 2017

shilman requested review from alexandrebodin and usulpro June 17, 2017 12:57

Update cra-kitchen-sink to use demo imports from @storybook/react

ed6d404

shilman added the cleanup Minor cleanup style change that won't show up in release changelog label Jun 17, 2017

shilman requested a review from ndelangen June 17, 2017 13:17

usulpro previously approved these changes Jun 17, 2017

View reviewed changes

alexandrebodin approved these changes Jun 17, 2017

View reviewed changes

shilman added 2 commits June 17, 2017 23:49

Move demo components from @storybook/components to @storybook/react

600b9bd

Minor cleanup

83e9977

Remove demo components from @storybook/components

0ab1de3

Add a file to @storybook/components so that the build doesn't break

02f39b9

alexandrebodin reviewed Jun 17, 2017

View reviewed changes

usulpro reviewed Jun 17, 2017

View reviewed changes

ndelangen approved these changes Jun 19, 2017

View reviewed changes

Merge branch 'master' into 1266-fix-components-deps

37141d6

shilman mentioned this pull request Jun 21, 2017

addon-knobs: Invalid prop knob.value of type object supplied to ObjectType, expected string #1292

Closed

tmeasday requested changes Jun 21, 2017

View reviewed changes

Remove unnecessary @storybook/react dep in @storybook/react-native

c7f7e07

Merge branch 'master' into 1266-fix-components-deps

d04f223

shilman and others added 8 commits June 22, 2017 10:54

Add placeholder tests to react-kitchen-sink

b6de6ac

Add package-lock.json

e5041fa

Per discussion with @tmeasday 😉

Use a pack -> install technique to recreate local packages

b04849b

As suggested by npm/npm#15900. This is a workaround until we get a better resolution of #1286

Added explanation of packs dir

77974e0

Revert removing @storybook/react from storyshots

45300c3

Merge pull request #1332 from storybooks/1331-test-cra-on-tarballs

d1b625a

Use a pack -> install technique to recreate local packages

Create @storybook/react/demo exports

f496deb

Merge branch 'master' into 1266-fix-components-deps

c61e045

shilman merged commit 84f6770 into master Jun 22, 2017

shilman added maintenance User-facing maintenance tasks and removed in progress cleanup Minor cleanup style change that won't show up in release changelog labels Jun 22, 2017

Hypnosphi deleted the 1266-fix-components-deps branch August 17, 2017 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Import demo components from @storybook/react #1303

Import demo components from @storybook/react #1303

shilman commented Jun 17, 2017 •

edited

Loading

codecov bot commented Jun 17, 2017 •

edited

Loading

usulpro left a comment

alexandrebodin left a comment

usulpro commented Jun 17, 2017

shilman commented Jun 17, 2017

shilman commented Jun 17, 2017

alexandrebodin left a comment

shilman commented Jun 17, 2017 •

edited

Loading

usulpro Jun 17, 2017

tmeasday Jun 21, 2017

usulpro Jun 21, 2017 •

edited

Loading

tmeasday Jun 21, 2017

ndelangen commented Jun 19, 2017

tmeasday commented Jun 21, 2017 •

edited

Loading

tmeasday Jun 21, 2017

tmeasday Jun 21, 2017

tmeasday Jun 21, 2017

shilman commented Jun 21, 2017 •

edited

Loading

shilman commented Jun 21, 2017

shilman commented Jun 22, 2017 •

edited

Loading

shilman commented Jun 22, 2017 •

edited

Loading

Import demo components from @storybook/react #1303

Import demo components from @storybook/react #1303

Conversation

shilman commented Jun 17, 2017 • edited Loading

What I did

How to test

codecov bot commented Jun 17, 2017 • edited Loading

Codecov Report

usulpro left a comment

Choose a reason for hiding this comment

alexandrebodin left a comment

Choose a reason for hiding this comment

usulpro commented Jun 17, 2017

shilman commented Jun 17, 2017

shilman commented Jun 17, 2017

alexandrebodin left a comment

Choose a reason for hiding this comment

shilman commented Jun 17, 2017 • edited Loading

usulpro Jun 17, 2017

Choose a reason for hiding this comment

tmeasday Jun 21, 2017

Choose a reason for hiding this comment

usulpro Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

tmeasday Jun 21, 2017

Choose a reason for hiding this comment

ndelangen commented Jun 19, 2017

tmeasday commented Jun 21, 2017 • edited Loading

tmeasday Jun 21, 2017

Choose a reason for hiding this comment

tmeasday Jun 21, 2017

Choose a reason for hiding this comment

tmeasday Jun 21, 2017

Choose a reason for hiding this comment

shilman commented Jun 21, 2017 • edited Loading

Update

shilman commented Jun 21, 2017

Update again

shilman commented Jun 22, 2017 • edited Loading

shilman commented Jun 22, 2017 • edited Loading

Triumph!

shilman commented Jun 17, 2017 •

edited

Loading

codecov bot commented Jun 17, 2017 •

edited

Loading

shilman commented Jun 17, 2017 •

edited

Loading

usulpro Jun 21, 2017 •

edited

Loading

tmeasday commented Jun 21, 2017 •

edited

Loading

shilman commented Jun 21, 2017 •

edited

Loading

shilman commented Jun 22, 2017 •

edited

Loading

shilman commented Jun 22, 2017 •

edited

Loading