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

Add cra-kitchen-sink and crna-getstorybook #1259

Closed
wants to merge 19 commits into from

Conversation

shilman
Copy link
Member

@shilman shilman commented Jun 12, 2017

Issue: #1220 #1218

What I did

Combined two broken PR's into one super-broken PR just so it's all in one place: #1219
#1224

  • renamed cra-storybook => cra-kitchen-sink
  • added notes, info, knobs, options, centered addon examples to cra-kitchen-sink.
  • renamed react-native-vanilla => react-native-getstorybook (which used to work on my machine, but no longer does)
  • added crna-getstorybook (which used to work on my machine, but no longer does)
  • changed react-native-getstorybook to use lerna bootstrapping

How to test

npm install && npm run boostrap
cd examples/<example>
npm run storybook

For react-native: react-native run-ios
For crna: npm run ios

What I need

  • move anything interesting from test-cra into cra-kitchen-sink
  • help with react-native

@codecov
Copy link

codecov bot commented Jun 12, 2017

Codecov Report

Merging #1259 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1259   +/-   ##
=======================================
  Coverage   13.73%   13.73%           
=======================================
  Files         207      207           
  Lines        4638     4638           
  Branches      513      601   +88     
=======================================
  Hits          637      637           
+ Misses       3557     3480   -77     
- Partials      444      521   +77
Impacted Files Coverage Δ
app/react-native/src/bin/storybook-start.js 0% <ø> (ø) ⬆️
app/react-native/src/bin/storybook-build.js 0% <ø> (ø) ⬆️
app/react-native/src/bin/storybook.js 0% <ø> (ø) ⬆️
...react-native/src/manager/components/PreviewHelp.js 0% <0%> (ø) ⬆️
addons/knobs/src/components/types/Color.js 8% <0%> (ø) ⬆️
addons/info/src/components/Props.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/routes.js 0% <0%> (ø) ⬆️
addons/storyshots/src/storybook-channel-mock.js 0% <0%> (ø) ⬆️
... and 28 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 a81283f...8d039e2. Read the comment docs.

@tmeasday
Copy link
Member

tmeasday commented Jun 14, 2017

Both of the test issues sound like npm problems to me:

https://travis-ci.org/storybooks/storybook/builds/241926871#L781:

Invariant Violation: addComponentAsRefTo(...): Only a ReactOwner can have refs. You might be adding a ref to a component that was not created inside a component's render method, or you have multiple copies of React loaded (details: https://fb.me/react-refs-must-have-owner).

This is typically when you have two copies of React, probably as a result of linking packages.

https://travis-ci.org/storybooks/storybook/builds/241926871#L812

Failed to find addon channel. This may be due to #1192.

I think this is due to multiple copies of addons, although this is a bit murkier. It's not happening for me locally with npm@4, and I can't install using npm@5 right now.


My feeling is the lerna approach is not working properly in terms of deduping.

@ndelangen
Copy link
Member

Can we just do away with the file dependencies all together then?

@tmeasday
Copy link
Member

tmeasday commented Jun 14, 2017

Can we just do away with the file dependencies all together then?

I'm not sure, I think previously (npm<5) they were solving these multiple loading problems because they installed our packages "properly" into test-cra. Now with npm@5/lerna, it's a symlink, which means (for instance) you get one copy of react installed in APP/node_modules/react and a second in APP/node_modules/@storybook/PKG/node_modules/react.

@tmeasday
Copy link
Member

@shilman RN is giving me serious headaches, let's split this back up into two and fix the non-RN stuff now.

@tmeasday
Copy link
Member

tmeasday commented Jun 15, 2017

Ok, I think I have diagnosed both problems. Ultimately (everything?) is due to the switch to npm@5 and the change to linking rather than "installing" local dependencies.

test-cra multiple addons require

Is due to this bug: jestjs/jest#3830

This basically means we cannot symlink packages and have Jest work with the current setup.

Recommendation: we disable running example app storyshot tests in CI (or anywhere :( ) until we resolve this (either a Jest fix/workaround or a non-linking solution).

(I also suggest we bring the storyshot tests into cra-kitchensink and remove test-cra).

RN example apps don't even run right now (not to mention tests)

facebook/react-native#637 - this leads to the "can't resolve package @storybook/react-native problem".

Ultimately this is due to the same problem, RN does not support symlinked node_modules.

Recommendation: we remove the RN apps until we resolve this (a non-linking solution).

@shilman
Copy link
Member Author

shilman commented Jun 15, 2017

Starting over to focus on cra-storybook only

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

Successfully merging this pull request may close these issues.

3 participants