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

Removing the packager from storybook. #4261

Merged
merged 3 commits into from
Oct 8, 2018
Merged

Conversation

Gongreg
Copy link
Member

@Gongreg Gongreg commented Oct 2, 2018

Issue: #4179

By having packager logic inside storybook we are prone for breaking things.
We have to support metro, haul, typescript and other cases. All of these things are responsibility of the user.

To avoid it I've completely removed packager logic from storybook.

What is more starting from v5.0 the default behaviour will be to skip the server altogether and include StorybookUI in your app so this is a first step to that direction.

I've added some info how to bring back old behaviour @petrbela suggested it.

@igor-dv
Copy link
Member

igor-dv commented Oct 2, 2018

I think it's rather an important breaking change, so having a note in the MIGRATION.md will be a good thing

@Gongreg
Copy link
Member Author

Gongreg commented Oct 2, 2018

@igor-dv Definitely.

@pksunkara pksunkara added this to the v4.0.0 milestone Oct 3, 2018
@codecov
Copy link

codecov bot commented Oct 7, 2018

Codecov Report

Merging #4261 into master will increase coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4261      +/-   ##
==========================================
+ Coverage   40.61%   40.77%   +0.15%     
==========================================
  Files         495      495              
  Lines        5875     5852      -23     
  Branches      784      777       -7     
==========================================
  Hits         2386     2386              
+ Misses       3111     3095      -16     
+ Partials      378      371       -7
Impacted Files Coverage Δ
app/react-native/src/bin/storybook-start.js 0% <ø> (ø) ⬆️

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 621a4ed...fdcfaf8. Read the comment docs.

try {
// eslint-disable-next-line global-require
require('babel-register')({
presets: [require.resolve('babel-preset-flow')],
Copy link
Member

Choose a reason for hiding this comment

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

do babel-register and babel-preset-flow still need to be installed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. What were they used for in the first place?

Copy link
Member

Choose a reason for hiding this comment

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

I have no idea =)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah but it is flow and some other things. I will remove it.
Thanks @igor-dv .

@igor-dv
Copy link
Member

igor-dv commented Oct 8, 2018

@Gongreg, is anything missing here ?

@Gongreg
Copy link
Member Author

Gongreg commented Oct 8, 2018

Nothing that I am aware of. We will do a blog post regarding this update. But it is not part of the PR

@igor-dv igor-dv merged commit d1cae5e into master Oct 8, 2018
@igor-dv igor-dv deleted the remove-packager-react-native branch October 8, 2018 21:20
@jqn
Copy link

jqn commented Oct 15, 2018

@igor-dv one thing that is missing from this is some instructions on how to switch from the storybook UI to the react native UI. The old behavior kept this separate but I don't see how this makes sense
module.exports = __DEV__ ? StorybookUI : App; if I want to switch back and forth. there has to be a better way to switch. Also if I'm missing something here please let me know.

@Gongreg
Copy link
Member Author

Gongreg commented Oct 15, 2018

@jqn You could have a storybook tab in bottom nav menu. Or inside admin panel. Or even the same switch you just wrote about. It shouldnt be more work to switch this way than killing packager and starting the new one. Or if you want to you can still start the packager old way, you just have to do it yourself. We will release a blogpost/update storybook website with information when 4.0 is released. Or you can check docs folder in @storybook/react-native code. (I am on mobile so it is hard to link it)

@Gongreg
Copy link
Member Author

Gongreg commented Oct 15, 2018

More or less to sum it up storybook should be part of your ui instead switching back and forth.

@kdawgwilk
Copy link

Or you can start the RN packager using the storybook dir: react-native start --projectRoot storybook

@kdawgwilk
Copy link

Actually I just tried to give this a go and with RN >= 0.56 this throws an error error: unknown option --projectRoot'`

@Gongreg
Copy link
Member Author

Gongreg commented Oct 15, 2018

It may be --projectRoots. From 0.57 it is --projectRoot. That is exactly the reason why we removed the packager. We don't want to maintain it.

@kdawgwilk
Copy link

It is --projectRoots but also giving a relative path doesn't work as expected and instead I need to give it a absolute path e.g.

--projectRoots `pwd`/storybook

@esbenvb
Copy link

esbenvb commented Oct 29, 2018

@kdawgwilk @Gongreg keep in mind that --projectRoot is not implemented in 0.57, but only documented. 0.57.1 fixes that
facebook/react-native@fb109e9

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.

6 participants