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

React native - Use Core #4942

Merged
merged 85 commits into from
Mar 15, 2019
Merged

React native - Use Core #4942

merged 85 commits into from
Mar 15, 2019

Conversation

igor-dv
Copy link
Member

@igor-dv igor-dv commented Dec 6, 2018

Issue: #4879 #4011 #4852 #4332 #5249 #5309

RN was a bit behind for a long time with all the new features we did. In order to support all of the good things, we should reuse as much as possible from the core. Now when the manager and preview are separated it's much easier to do.

@Gongreg, we can maybe continue the #4752 here or create a new branch from this one.
Issues we can also finish for the breaking v5 release:

What I did

  • moved src/manager and src/preview to src/client/* so it will be similar to other apps

  • deleted all the webpack/babel/config related code from rn/server and replaced it with the dev-server from core

  • changed dev-server to be able to run the webpack only for manager

  • used standalone api to build storybook in a dev mode

  • removed --secured param (-s) and used --https like in other apps

  • removed build command (it was not doing anything).

TBD:

  • Validate breaking changes
  • Check something very useful is not missing now
  • Check what to do now with the environment parameter
  • Reflect everything in docs
  • Update migration tutorial

@codecov
Copy link

codecov bot commented Dec 6, 2018

Codecov Report

Merging #4942 into next will increase coverage by 0.54%.
The diff coverage is 6.94%.

Impacted file tree graph

@@            Coverage Diff            @@
##            next    #4942      +/-   ##
=========================================
+ Coverage   37.1%   37.64%   +0.54%     
=========================================
  Files        648      642       -6     
  Lines       9524     9387     -137     
  Branches    1381     1334      -47     
=========================================
  Hits        3534     3534              
+ Misses      5411     5293     -118     
+ Partials     579      560      -19
Impacted Files Coverage Δ
lib/cli/bin/generate.js 0% <ø> (ø) ⬆️
lib/ui/src/provider.js 0% <ø> (ø) ⬆️
...erver/src/client/manager/components/PreviewHelp.js 0% <ø> (ø)
...pp/react-native-server/src/client/manager/index.js 0% <ø> (ø)
addons/ondevice-knobs/src/panel.js 0% <0%> (ø) ⬆️
app/react-native-server/src/server/index.js 0% <0%> (ø)
...react-native-server/src/client/manager/provider.js 0% <0%> (ø)
app/react-native/src/preview/index.js 0% <0%> (ø) ⬆️
app/react-native-server/src/server/cli.js 0% <0%> (ø)
lib/ui/src/core/init-provider-api.js 0% <0%> (ø) ⬆️
... and 12 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 5175731...d3f6663. Read the comment docs.

@storybook-safe-bot
Copy link
Contributor

storybook-safe-bot commented Dec 6, 2018

Fails
🚫 PR is marked with "do not merge", "in progress", "BREAKING CHANGE" labels.
🚫

Please choose only one of these labels: ["BREAKING CHANGE","feature request"]

Generated by 🚫 dangerJS against af3a4d5

@igor-dv
Copy link
Member Author

igor-dv commented Dec 7, 2018

Currently, when running an app with expo I am getting the babe6/babel7 mismatch...

@igor-dv
Copy link
Member Author

igor-dv commented Dec 7, 2018

upgrading to sdk31 looks solving the babel issue, but there is something new now =/

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.

So happy this is almost mergeable. Some minor nitpicks above, but otherwise it looks great to me.

addons/ondevice-knobs/src/panel.js Outdated Show resolved Hide resolved
app/react-native-server/readme.md Outdated Show resolved Hide resolved
app/react-native/package.json Outdated Show resolved Hide resolved
app/react-native/readme.md Show resolved Hide resolved
app/react-native/src/preview/index.js Outdated Show resolved Hide resolved
app/react-native/src/preview/index.js Outdated Show resolved Hide resolved
examples-native/crna-kitchen-sink/package.json Outdated Show resolved Hide resolved
@@ -22,6 +22,7 @@ if (process.argv[1].includes('getstorybook')) {
.option('-N --use-npm', 'Use npm to install deps')
.option('-p --parser <babel | babylon | flow>', 'jscodeshift parser')
.option('-t --type <type>', 'Add Storybook for a specific project type')
.option('-y --yes', 'Answer yes to all prompts')
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a separate PR?

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.

Awesome!!!

@ndelangen ndelangen merged commit 7fbb406 into next Mar 15, 2019
@ndelangen ndelangen deleted the react-native/use-core-for-server branch March 15, 2019 09:44
@igor-dv
Copy link
Member Author

igor-dv commented Mar 15, 2019

Oops I am late. Yay !! Great Success 💥

@dangreenisrael
Copy link
Member

🎉

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.