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

Added support for the haul react-native packager #1294

Merged
merged 5 commits into from
Jun 16, 2017

Conversation

ericwooley
Copy link
Contributor

Issue: New Feature

What I did

I added another cli option to use the haul cli instead of the react-native packager.

How to test

  1. create a react-native new app with a new install of storybook.

  2. install haul from hauls app initializer.

  3. cp webpack.haul.js webpack.haul.storybook.js

  4. change the entry from

module.exports = ({ platform }) => ({
  entry: `./index.${platform}.js`,
});

to

module.exports = ({ platform }) => ({
  entry: `./storybook/index.${platform}.js`,
});
  1. add --haul webpack.haul.storybook.js to your storybook command
  2. run storybook.

Haul should take over and do the job of the react-native packager.

@shilman
Copy link
Member

shilman commented Jun 16, 2017

Nice one, thanks @ericwooley ! PR looks good, I'll test it out later today and let you know if I have questions.

@codecov
Copy link

codecov bot commented Jun 16, 2017

Codecov Report

Merging #1294 into master will decrease coverage by 0.05%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1294      +/-   ##
==========================================
- Coverage   13.76%   13.71%   -0.06%     
==========================================
  Files         201      207       +6     
  Lines        4620     4646      +26     
  Branches      492      497       +5     
==========================================
+ Hits          636      637       +1     
- Misses       3570     3588      +18     
- Partials      414      421       +7
Impacted Files Coverage Δ
app/react-native/src/bin/storybook-start.js 0% <0%> (ø) ⬆️
addons/knobs/src/components/types/Color.js 8% <0%> (ø) ⬆️
...dons/storyshots/stories/directly_required/index.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/left_panel.js 20% <0%> (ø) ⬆️
.../ui/src/modules/ui/components/layout/dimensions.js 15.62% <0%> (ø) ⬆️
lib/ui/src/modules/api/configs/init_api.js 40.47% <0%> (ø) ⬆️
addons/info/src/components/PropTable.js 0% <0%> (ø) ⬆️
...rc/modules/ui/components/left_panel/text_filter.js 33.33% <0%> (ø) ⬆️
lib/ui/src/modules/shortcuts/actions/shortcuts.js 6.25% <0%> (ø) ⬆️
...src/server/config/WatchMissingNodeModulesPlugin.js 0% <0%> (ø) ⬆️
... and 22 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 40858bf...58a11ec. Read the comment docs.

Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

PR looks good to me, @shilman this sounds like a breakthrough in regards to the RN + npm linking problem?


[Haul](https://github.com/callstack-io/haul) is an alternative to the react-native packager and has several advantages in that it allows you to define your own loaders, and handles symlinks better.

If you want to use haul instead of the react-native packager, modify they storybook npm script too:
Copy link
Member

Choose a reason for hiding this comment

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

Typo: too to

Copy link
Member

Choose a reason for hiding this comment

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

And they the

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm so bad with spelling.

I need some kind of ai to just point this kinda stuff out. If only there were some kind of build in tools...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I know this pain, for one of my blogs we use danger-prose to lint for typos.

@shilman
Copy link
Member

shilman commented Jun 16, 2017

@ndelangen @tmeasday YES! haul does solve our RN simlink problem. tested it out for this PR, and also with lerna bootstrap. great news and thank you @ericwooley for introducing us to haul. will be opening a follow-on PR to integrate it into our build process 🚀

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.

@ericwooley this is super awesome. mind fixing those minor typos that @orta pointed out? then i'll merge.

@shilman
Copy link
Member

shilman commented Jun 16, 2017

Nevermind. Made the changes myself since I'm so eager to get this merged 😄

@shilman shilman changed the title add haul support Added support for the haul react-native packager Jun 16, 2017
@shilman shilman merged commit f3b0636 into storybookjs:master Jun 16, 2017
@ericwooley
Copy link
Contributor Author

ericwooley commented Jun 16, 2017

@shilman @ndelangen I'm glad you like it! I was having lerna issues as well which motivated me to investigate!

An interesting thing, after I started using my fork. Stories imported from node_modules/my_linked_shared_components/stories.js do not show up in the list of stories. I suspect they are on some kind of ignore list.

Can anyone point me in the direction of where such code might be? I'll open another PR if I can pinpoint the issue.

This actually maybe related to #1068 , will investigate more.

@ericwooley ericwooley deleted the haul-support branch June 16, 2017 16:24
@slorber
Copy link
Contributor

slorber commented Jun 17, 2017

great!

Now maybe it's worth to add this to the contribution guide, and maybe create a "test-crna" example that automatically has the proper haul config already setup so that contributors can easily test their changes locally?

@shilman
Copy link
Member

shilman commented Jun 17, 2017

That's the plan. I'm offline for the next four days but can pick it up when I get back. Or feel free to pick it up and run with it!

@tmeasday
Copy link
Member

tmeasday commented Jun 19, 2017

Such good news! Do we need to add haul as a dependency?

@SEAPUNK
Copy link

SEAPUNK commented Jun 20, 2017

Doesn't Expo use the react-native packager regardless, though? How does the packaging process go in correlation to running Storybook and Expo w/ storybook start and then react-native-scripts ios?

@ericwooley
Copy link
Contributor Author

@tmeasday currently I don't think you would, but I am not certain.
If they user is switching storybook to use haul, they are almost certainly using haul for their app and thus it should be installed.

There may be some edge case (that I can't think of) where a user wants to use haul for their storybook, and the react-native packager for the actual app, but if thats they case I think they will have to live with adding haul as a dependency manually, instead of requiring everyone install the haul cli, just in case.

@SEAPUNK I'm not super familiar with how expo does it. If they just run npm run storybook then I would think it should be fine. If they run a custom command via ./node_modules/.bin/...whatever, i don't think this would work.

@ericwooley
Copy link
Contributor Author

@tmeasday If there is an argument added to getStorybook, eg getStorybook --use-haul it could add that as a dev dependency automatically, and setup the haul webpack file?

@tmeasday
Copy link
Member

@ericwooley -- I'm not sure what's best, but perhaps we should add it as an optionalDependency at the least and show some kind of error if it's not installed? It feels weird/wrong to import something that's not in package.json.

@shilman
Copy link
Member

shilman commented Jun 21, 2017

@tmeasday @ericwooley good point about the dependency. i totally missed this in my review. i was too excited about fixing our development/testing environment. 😄

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.

7 participants