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

Bump react-native-vanilla example to 0.57 #4196

Closed
wants to merge 1 commit into from

Conversation

oblador
Copy link
Member

@oblador oblador commented Sep 18, 2018

Current react-native-vanilla example is not compiling and is out of date. Further it needs to be updated to illustrate issues with the new version (separate PR coming soon) as outlined in #4179.

What I did

Generated a new project from scratch using the latest version of React Native: 0.57.

How to test

Since Storybook is broken with the latest RN so is this PR, but if you want to test it out it can be done like this:

$ yarn storybook
$ react-native start
$ yarn start

@oblador oblador added maintenance User-facing maintenance tasks dependencies:update labels Sep 18, 2018
@ndelangen
Copy link
Member

@Gongreg thoughts?

@ndelangen
Copy link
Member

Hey @oblador I spoke to @Gongreg about this.

He's on vacation and besides spending time away from the keyboard (very healthy) he's also not able to log in to github on his current device, so I'm relaying a message:

I want to remove that example as soon as I can, remove packager and separate out the server.
So these things will be obsolete.

For users using older react native with babel 6 they will be able to use the server as a separate separate project if necessary.

Do you think it's meaningful to merge this meanwhile @oblador ?

If yes, I'll gladly merge it.

I think @Gongreg could use someone to pair with about Storybook for RN, are you on slack/discord? You 2 should get in contact after his vacation about this.

@kdawgwilk
Copy link

This is broken due to the changes made here:
facebook/react-native@c5ce762#diff-4a892a6b8856f0253e2de7094e585ae6 and here facebook/react-native@33ba5e8#diff-4a892a6b8856f0253e2de7094e585ae6

That changed the arguments from --projectRoots [list] -> --watchFolders [list] and --root [list] -> --projectRoot [string]

@oblador
Copy link
Member Author

oblador commented Sep 18, 2018

@kdawgwilk I know, and I mention it in the description; my plan is to attack it in a separate PR. However that diff also broke those new arguments, but I have a PR up for that: facebook/react-native#21165

@ndelangen I'm on slack and have talked to him before. This PR is mostly preparation for the mentioned incompatibility with RN 0.57. I guess I could fix it regardless, but this way we could at least manually test it in lieu of automated tests.

As a sidenote, my impression is that RN is a bit forgotten by the overall team/community and has regressions on a regular basis. Maybe we should discuss at least having some kind of basic smoke test to get rid of the worst of it?

@ndelangen
Copy link
Member

@oblador YES that'd be fantastic! We use circleCI and they do provide macOS runners if we need that.

We could use OpenCollective money to pay for it, or ask CircleCI for sponsorship

@Gongreg
Copy link
Member

Gongreg commented Sep 19, 2018

Hey @oblador .
We are going to remove the packager from RN storybook.

It is not storybook responsibility to know what packager you want to use.

I think that would even make more sense to do now than later so people would know that you can use rn storybook not only by running storybook as a command but you can include it inside your app and start server (if neccessary) with --skip-packager command.

@Gongreg
Copy link
Member

Gongreg commented Sep 19, 2018

Also I want to leave only one example for RN storybook, preferably expo since it is more beginner friendly to use. Nobody is maintaining the vanilla one.
I would really like to add some tests for it, just have other priorities before that.

@oblador
Copy link
Member Author

oblador commented Sep 19, 2018

@Gongreg OK, fair enough. Does that mean that I shouldn't submit a PR with the new flags?

@AugustoAleGon
Copy link

I hope the merge will be released soon. I am currently facing an error probably the PR will help me.

@Gongreg
Copy link
Member

Gongreg commented Sep 19, 2018

Will updating the arguments break versions older than RN 0,57? If it doesnt it is definitely worth to push it. Otherwise I am not sure since most people dont migrate to latest RN version as soon as possible. I wont be able to do anything for few weeks so my changes will have to wait. I think @oblador it is your call now.

@oblador
Copy link
Member Author

oblador commented Sep 20, 2018

@Gongreg It can be done in a backwards compatible manner if we do some inspection of the react-native version, but it's somewhat slow, adds another second or two. I think I will go with the --no-packager route myself, but maybe we should communicate that somehow.

@Gongreg
Copy link
Member

Gongreg commented Sep 20, 2018

@oblador , I've been trying to document it on my PR. @ndelangen do you have ideas how we could document this issue and notify people about --no-packager option for the time I am away?

@AugustoAleGon
Copy link

What is the no-package option? @Gongreg .

@Gongreg
Copy link
Member

Gongreg commented Sep 20, 2018

I think it is actually --skip-packager.

It starts storybook server without packager so you can reuse same packager instance you use while developing your app. Then you just render storybook component anywhere in your app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies maintenance User-facing maintenance tasks react-native
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants