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

CRA_BABEL_PRESET_FILE to handle optional dependencies. #2775

Closed

Conversation

jsdevel
Copy link

@jsdevel jsdevel commented Jul 12, 2017

  • This will allow for optional babel dependencies to be added without ejecting cra.
  • CRA_BABEL_PRESET_FILE=relative/path/to/babel/preset can be added to package.json.

I verified this locally by copying index.js to node_modules/babel-preset-react-app/index.js within one of my projects. Here is the package.json for my project:

{
  "name": "my-app",
  "version": "0.1.0",
  "private": true,
  "dependencies": {
    "babel-plugin-relay": "^1.1.0",
    "react": "^15.6.1",
    "react-dom": "^15.6.1",
    "react-router-dom": "^4.1.1",
    "react-scripts": "1.0.10",
    "shared-git-hooks": "^1.2.1"
  },
  "scripts": {
    "start": "CRA_BABEL_PRESET_FILE=.babel-preset-react-app react-scripts start",
    "build": "react-scripts build",
    "test": "react-scripts test --env=jsdom",
    "eject": "react-scripts eject"
  },
  "homepage": "."
}

I'm doing this because I want to use relay modern in my app. My .babel-preset-react-app.js file is exactly what @maletor had in PR #2343

cc @gaearon Is this an acceptable approach per your comment #2343 (comment) ?

Also relates to #2001

@maletor
Copy link

maletor commented Jul 13, 2017

This looks promising.

@jsdevel jsdevel force-pushed the optional-babel-dependencies branch 2 times, most recently from dfbe775 to c143cb3 Compare July 13, 2017 22:08
* This will allow for optional babel dependencies to be added without ejecting cra.
* CRA_BABEL_PRESET_FILE=relative/path/to/babel/preset can be added to package.json.
@jsdevel jsdevel force-pushed the optional-babel-dependencies branch from c143cb3 to f7bba1d Compare July 13, 2017 22:10
@jsdevel
Copy link
Author

jsdevel commented Jul 13, 2017

This looks promising.

Thanks! I also updated the documentation for Advanced Configuration.

@Timer
Copy link
Contributor

Timer commented Jul 13, 2017

Unfortunately, I do not see this as a suitable solution -- this is effectively letting someone gain full control over the babel preset and allows them to break certain things or use a stage-0 preset; something we are highly against.

I appreciate your work, however, @jsdevel.

@Timer
Copy link
Contributor

Timer commented Jul 13, 2017

Right now, I would love some feedback on #2784 -- a Relay plugin would be great!

@jsdevel
Copy link
Author

jsdevel commented Jul 13, 2017

@Timer thanks for the feedback. I spent time mulling over #2784 before introducing this as an alternative. Really my main reason for introducing this hinged around @gaearon 's comment #2343 (comment) which stated:

We need some plan for a bunch of optional dependencies people want: Sass, Relay, TypeScript. These are all in principle possible to support but we don't want to either create a second plugin system (which webpack already "is") ...

When I saw that I realized that we probably just need a simply way of extending CRA for those who would rather not eject, but are comfortable maintaining certain parts.

@Timer
Copy link
Contributor

Timer commented Jul 13, 2017

Managing certain parts of the configuration falls outside of our core philosophy, even if the user(s) are comfortable with doing so.

It breaks what people love about updating react-scripts, which is as simple as bumping the version (not having to worry about incompatibilities, updating extensions, etc).

For example, when we upgrade to Babel 7, many users will experience breakages if they use a custom babel preset.

They may rely on presets which were not updated to have Babel 7 support. Or they may find themselves using a stage preset which will change wildly in the next Babel release.

Since there's no merging of the babel preset either, many people will lag behind when we upgrade our Babel preset like we have recently, e.g.:

  1. Disabled CommonJS transform
  2. Added import() support

We may be adding other plugins in the future:

  1. idx
  2. babel-macros

If people are comfortable taking full control over the Babel Configuration, they may as well eject so things don't break unexpectedly for them.

@jsdevel
Copy link
Author

jsdevel commented Jul 14, 2017

No rules are perfect. Sometimes we may introduce flags or configuration if we believe the value is high enough to justify the mental cost

To me this is one of those cases. Most users aren't likely to regularly update react-scripts, ejected or not. If they do then they understand that certain things might break and need refactoring afterwards.

@jsdevel
Copy link
Author

jsdevel commented Jul 14, 2017

Since there's no merging of the babel preset either, many people will lag behind when we upgrade our Babel preset like we have recently, e.g.:

Isn't that the cost of updating any framework?

@jsdevel
Copy link
Author

jsdevel commented Jul 14, 2017

Either way, I thought this proposal was easy enough to put together to warrant the conversation. I'll be perfectly happy if it gets merged or abandoned. CRA is worth it 😂

@Timer
Copy link
Contributor

Timer commented Jul 14, 2017

To me this is one of those cases. Most users aren't likely to regularly update react-scripts, ejected or not. If they do then they understand that certain things might break and need refactoring afterwards.

Really? As far as I know people update react-scripts constantly.
We almost never have something break that requires refactoring; but this would be the case if you controlled your own Babel config.

Isn't that the cost of updating any framework?

Not our framework. 😛 We aim to be as painless as possible; providing explicit migration instructions for any possible breakages (which only happen in seldom major version releases).

@Timer
Copy link
Contributor

Timer commented Aug 2, 2017

Closing per our discussion; sorry! We're just not ready to allow this sort of configuration yet.

Check out Neutrino or react-app-rewired!

@Timer Timer closed this Aug 2, 2017
@jsdevel jsdevel deleted the optional-babel-dependencies branch August 2, 2017 03:57
@lock lock bot locked and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants