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

CLI Project Generator: Explicitly include a default .babelrc #7822

Closed
wants to merge 4 commits into from

Conversation

namuol
Copy link

@namuol namuol commented May 29, 2016

Motivation

React Native projects nested inside babel-enabled projects can break unexpectedly due to misconfigured .babelrc in the parent project.

See #7821 for details.

Test plan

Added a simple check to ensure the generator provides a .babelrc file by default.

@ghost
Copy link

ghost commented May 29, 2016

By analyzing the blame information on this pull request, we identified @ide and @skevy to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels May 29, 2016
@janicduplessis
Copy link
Contributor

Tests are failing on node 4 (npm 2). I think you have to add "babel-preset-react-native" to package.json for the generated project. It works on npm 3 because of flattened dependencies.

@ghost
Copy link

ghost commented May 29, 2016

@namuol updated the pull request.

@ghost
Copy link

ghost commented Jun 1, 2016

@namuol updated the pull request.

@ghost
Copy link

ghost commented Jul 1, 2016

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @bestander as a potential reviewer. Could you take a look please or cc someone with more context?

@bestander
Copy link
Contributor

babel-preset-react-native may become incompatible with the one react-native is using when the project gets updated so I am not 100% happy about adding a new peer dependency in react-native-cli.

Can we do to babel-preset-react-native the same thing we do to react on local-cli?

@bestander
Copy link
Contributor

Sorry for delay :)

@ghost ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 12, 2016
@mkonicek
Copy link
Contributor

mkonicek commented Sep 9, 2016

Based on @bestander's last comment I think we should close this and revisit later if needed.

Thanks for the contribution and caring about improving tests!

@mkonicek mkonicek closed this Sep 9, 2016
@tgoldenberg
Copy link
Contributor

So after looking at the related issue #7821, I think there must be an easy solution here. It sounds like @mkonicek doesn't want to modify the peer dependencies on initialization, so here's what I propose:

What if we keep the default .babelrc file with {"presets": ["react-native"] }, but not include a peer dependency of babel-preset-react-native? The caveat to this is that it would throw an error on npm 2, but couldn't we also make a minimum version of npm to use, and include that in the Getting Started docs?

I don't think the issue should be ignored, though. There is precedent in create-react-app, for example, of addressing the issue of confusing behavior due to babel searching parent directories (link).

Sorry if I'm being naive here, this is the first RN issue I've actively worked on. I also have a forked version without the peer dependency changes at https://github.com/tgoldenberg/react-native. @janicduplessis @namuol what do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants