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

A workaround to use latest JS features without ejecting, is this considered bad practice? #2912

Closed
sonaye opened this issue Aug 6, 2017 · 16 comments

Comments

@sonaye
Copy link

sonaye commented Aug 6, 2017

Take a look at this command/script:

rm -rf src && babel dev -w -d src --presets=es2015,react,stage-0 --plugins=transform-decorators-legacy --copy-files

We are telling Babel to:

  • Initialize an empty src directory, this is the main CRA src we are used to.
  • Watch for any changes in all JS files located in the dev directory, and compile accordingly (compiled version of all dev is now located in src).
  • We are interested in supporting some presets (es@2015, react, es@next).
  • We are interested in having some plugins/additional functionality (decorators).
  • Replicate all non JS files in dev, now we have an exact replicated and compiled version of our app in src.
app/
-- dev/ .. where all development happens, extended with all the new JS features
-- build/ .. contains final build of the app
-- src/ .. contains compiled JS code generated by Babel

Translating this idea via package.json:

{
  "devDependencies": {
    "babel-cli": "6.24.1",
    "babel-plugin-transform-decorators-legacy": "1.3.4",
    "babel-preset-es2015": "6.24.1",
    "babel-preset-react": "6.24.1",
    "babel-preset-stage-0": "6.24.1"
  },
  "scripts": {
    "build": "react-scripts build",
    "start": "react-scripts start",
    "start:babel": "rm -rf src && babel dev -w -d src --presets=es2015,react,stage-0 --plugins=transform-decorators-legacy --copy-files"
  }
}

When running yarn start in parallel with yarn start:babel, we get the desired result.

Pros

  • Use the latest JS features without having to eject. This is not the place to discuss whether we should or shouldn't, let's just take it for granted for now that we are interested in using the latest features available.

Cons

  • Limited linting and type checking experience with ESLint and Flow, (any additional features added and not supported by react-scripts would typically throw either a warning or an error), we are unable to utilize these tools to their fullest potential.
  • Limited debugging experience, I would say it's almost non existent now because of the extremely verbose compiled code, you get the high level error detection abilities only.
  • CRA compiles an already compiled code now? how bad is this?

Would love to hear your thoughts on this.

@Timer
Copy link
Contributor

Timer commented Aug 6, 2017

I don't see anything immediately wrong with this other than the cons you listed.

You are, however, exposing yourself to breakages and ecosystem churn by using stage-0 features. This is considered very poor practice for a non-side-project.

We always enable the "latest JS features", remember anything not stage 4 is not really JavaScript. It's a proposal -- we do enable proposals, but only those in stage 3.

@sonaye
Copy link
Author

sonaye commented Aug 6, 2017

Stage 0 was just an example, typically one would go with stage 2 or stage 3 or just cherry pick.

I've been developing with a nice set of less-than-stage-3 features for over a year now in React Native, I reached a point where I just cannot truly be productive without utilizing them. Take these two examples which are both at stage 2 at the moment.
https://babeljs.io/docs/plugins/transform-decorators/
https://babeljs.io/docs/plugins/transform-class-properties/

e.g. I didn't have to write .bind(this) for over a year now, and a couple of days ago I found myself googling the different methods to achieve that, there is just no way that I am going back to those ol' days 😅

It is similar to going back to building web apps with jQuery instead of React, it's just not the same experience, and that's just the beginning of a very long list of arguments. Language can be -and is- a true enabler.

I am hoping that maybe someone already went through this and could possibly provide some input on it.

@Timer
Copy link
Contributor

Timer commented Aug 6, 2017

We support class properties, just not decorators. The decorators spec implemented by babel has been abandoned and replaced AFAIK; we should see movement on this front with the Babel 7 release.

This proposal is still stage 2 so we aren't considering it yet; probably when they go stage 3.

@gaearon
Copy link
Contributor

gaearon commented Aug 6, 2017

IMO this is making the setup more complicated than ejecting in the first place.
But if that works for you, I guess it’s fine. :-)

@gaearon gaearon closed this as completed Aug 6, 2017
@sonaye
Copy link
Author

sonaye commented Aug 7, 2017

@Timer Are you sure? Writing something like this would throw for me.

export default new class {
  foo = 'bar'; // this.foo is undefined
  baz = () => new Date().getTime(); // this.baz() is undefined
}(); 

@sonaye
Copy link
Author

sonaye commented Aug 7, 2017

@gaearon Tried ejecting, I disagree, too much exposed variables that I would rather a tool takes care of them for me, CRA is doing an excellent job in this area. One question though that I couldn't find a straightforward answer to was "What do I lose when ejecting from CRA", any info on that?

@Timer
Copy link
Contributor

Timer commented Aug 7, 2017

@sonaye switch to NPM 4, wipe out your node_modules, and reinstall. An NPM 5 bug causes ESLint to think that syntax is invalid.

@sonaye
Copy link
Author

sonaye commented Aug 7, 2017

I am still using npm 4.6.1, react-scripts 1.0.10.

screen shot 2017-08-08 at 1 58 11 am

@Timer
Copy link
Contributor

Timer commented Aug 8, 2017

That's definitely a bug! It works on my system.

Can you provide a repro?

@sonaye
Copy link
Author

sonaye commented Aug 11, 2017

I just tested this on a fresh CRA app and no errors were thrown, after further investigation my guess is that this was ESLint related and not CRA.

@gaearon
Copy link
Contributor

gaearon commented Aug 15, 2017

I would like to point out that the decorators spec has changed significantly. I don't recommend anyone to rely on the current legacy transform. See this thread for more info: https://twitter.com/dan_abramov/status/897491076537356288

@sonaye
Copy link
Author

sonaye commented Aug 31, 2017

I just want to document that I have personally moved to what I think is a better solution since then, react-app-rewired.

const { injectBabelPlugin } = require('react-app-rewired');

module.exports = override = config => {
  config = injectBabelPlugin('transform-decorators-legacy', config);
  config = injectBabelPlugin('transform-export-extensions', config); // no flow support yet

  // getting eslint to work is a bit tricky but can be achieved
  // config.module.rules[0].use[0].options.useEslintrc = true;

  return config;
};
"devDependencies": {
  "babel-plugin-transform-decorators-legacy": "1.3.4",
  "babel-plugin-transform-export-extensions": "6.22.0",
  "react-app-rewired": "1.2.4",
  "react-scripts": "1.0.12"
},
"scripts": {
  "build": "react-app-rewired build",
  "start": "react-app-rewired start"
}

Coming back to my list of cons.

  • Limited linting and type checking experience with ESLint and Flow, (any additional features added and not supported by react-scripts would typically throw either a warning or an error), we are unable to utilize these tools to their fullest potential.
  • Limited debugging experience, I would say it's almost non existent now because of the extremely verbose compiled code, you get the high level error detection abilities only.
  • CRA compiles an already compiled code now? how bad is this?

Hopefully we can see something like this within CRA in the near future, related to #670.

@gaearon
Copy link
Contributor

gaearon commented Aug 31, 2017

Well, again, I think not enabling decorators has been a good call given that the new specification version, just like I said, is incompatible.

Nevermind, just realized I already wrote this in the above comment :-)

My point is decorators are not a "latest feature". They are a spec with an old legacy version that will never be supported by the browsers (and which you use), and a new version that has not been implemented by Babel (and which is incompatible with your existing code). This is a footgun. Even if we implement a plugin system we won't let users depend on an abandoned proposal (even if they find it useful). It's just a net loss for the ecosystem due to all the confusion and pain it causes.

@sonaye
Copy link
Author

sonaye commented Aug 31, 2017

@gaearon I completely understand your point, and I do agree to a great extent. I am not advocating for using decorators in any way, but simply documenting/offering a solution/workaround for those who still -or choose to- use the legacy transformation or any other transformations for that matter that are not backed by CRA. At the end of the day, we are responsible for it and moved forward knowing that it's still a proposal. At this stage, legacy decorators are still very valuable for me to be dropped from my stack, until a good replacement is found out there at least.

I don't agree however with your point about not giving the developers the freedom to choose or build their own plugins -when that a system is available-, this decision should be left to the developers in my opinion, given that they are prompted that no support is provided for non-CRA-packed plugins. I wouldn't call it a plugin system if only whitelisted add-ons are allowed, I am following the progress here btw #2784.

CRA is wonderful because it offers zero configuration out of the box, this is extremely powerful, I would even call it "revolutionary" because I've built with React before CRA. The range between 0 configs and all the configs is massive, the current workaround I showcased is for those who still don't want to go all the way to the extreme end of the range (by ejecting) and can still tweak minor flags that are located at the beginning of this range.

You are the maintainers of this project, and whatever decisions you make I will respect and support at the end, and that's not just because you do have a valid point behind it too, which I completely understand its motives, we make different design decisions, that's all :)

@gaearon
Copy link
Contributor

gaearon commented Sep 1, 2017

Fair enough! :-)

@raymondsze
Copy link

@sonaye
Just a suggestion.

I created a library called 'create-react-scripts', it use the similar manner as what "react-app-rewired" did but you are able to re-publish it like your version of react-scripts which "react-app-rewired" not allow you to do so.

To enable the decorator support,
You could make use of "create-react-scripts" and "create-react-scripts-babelrc" to enable the babelrc option of babel-loader, so you can override the babel configuration and you still enjoy all things create-react-app provides.

I am still a newbie on open source community, looking for feedback.
https://github.com/raymondsze/create-react-scripts

@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.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants