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

Fix babel-preset-env. #765

Merged
merged 3 commits into from
Jan 25, 2018
Merged

Fix babel-preset-env. #765

merged 3 commits into from
Jan 25, 2018

Conversation

tech4him1
Copy link
Contributor

- Summary

We added babel-preset-env before, but I was using the docs for the beta version instead of the released version, so it was actually still transpiling everything instead of just the stuff we needed.

- Test plan

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

Copy link
Contributor

@erquhart erquhart left a comment

Choose a reason for hiding this comment

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

👏

webpack.prod.js Outdated
@@ -3,6 +3,7 @@ const path = require('path');
const webpack = require('webpack');
const merge = require('webpack-merge');
const ExtractTextPlugin = require('extract-text-webpack-plugin');
const UglifyJSPlugin = require('uglifyjs-webpack-plugin');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be UglifyJsPlugin (lowercase "s").

Copy link
Contributor Author

@tech4him1 tech4him1 Nov 8, 2017

Choose a reason for hiding this comment

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

Yup, already figured that out, thank you. This PR in general is throwing some really weird errors about components not being mounted (after this line is fixed), please check it out if you get a chance.

@tech4him1
Copy link
Contributor Author

OK, here is the problem with React Hot Loader: gaearon/react-hot-loader#695.

@tech4him1 tech4him1 force-pushed the fix-es6 branch 2 times, most recently from 9ffa3b6 to fbdcb3f Compare December 8, 2017 21:11
@tech4him1 tech4him1 changed the title WIP: Fix babel-preset-env. Fix babel-preset-env. Dec 8, 2017
@tech4him1
Copy link
Contributor Author

React Hot Loader was removed in 1.0, so this should be good to merge now. We'll need to consider this if we ever add React Hot Loader in again, but they are working on fixing the problem, so hopefully it will be resolved by then.

@verythorough
Copy link
Contributor

verythorough commented Dec 20, 2017

Deploy preview for cms-demo ready!

Built with commit cd3e623

https://deploy-preview-765--cms-demo.netlify.com

@tech4him1
Copy link
Contributor Author

@erquhart @Benaiah Rebased on 1.0.3 and ready for review.

@tech4him1
Copy link
Contributor Author

@erquhart already reviewed this, do you want me to just merge?

@erquhart
Copy link
Contributor

Yep, go for it

@erquhart
Copy link
Contributor

Added node 6 as a target, should pass now.

@tech4him1
Copy link
Contributor Author

@erquhart Should we just upgrade our test environment?

@erquhart
Copy link
Contributor

erquhart commented Jan 24, 2018

Keeping it this way allows contributors with Node 6 to still run the test suite. I don't believe this affects the build output - if it does, upgrading is worth considering.

@tech4him1 tech4him1 changed the title WIP: Fix babel-preset-env. Fix babel-preset-env. Jan 24, 2018
@tech4him1
Copy link
Contributor Author

tech4him1 commented Jan 24, 2018

@erquhart Hmm, Node 6 is still supported, so I guess we could. Just doesn't make very much sense to me to transpile stuff just for our test environment when all the browsers support it. I wouldn't want to do it on a conditional, though, that could cause more problems (tests pass, in browser doesn't).

@erquhart
Copy link
Contributor

That's what I don't know - are we transpiling for node in the build or just on the fly during testing?

@erquhart
Copy link
Contributor

It appears this does impact the build. Odd.

Also, it appears we're now in a parallel universe where browsers support pretty much everything natively?

screen shot 2018-01-24 at 12 17 36 pm

Honestly, I'd hate to require Node 7+. Performance matters, but we're not exactly dying in that department. I vote we open a separate issue about it and keep this one focused on using babel-preset-env.

@tech4him1
Copy link
Contributor Author

tech4him1 commented Jan 24, 2018

I'm good with that, feel free to merge.

Here is what we are currently transpiling with babel-preset-env, and why:

Using plugins:
  transform-es2015-destructuring {"edge":"15","node":"6"}
  transform-es2015-for-of {"node":"6"}
  transform-es2015-function-name {"edge":"15","node":"6"}
  transform-exponentiation-operator {"node":"6"}
  transform-async-to-generator {"node":"6"}
  syntax-trailing-function-commas {"node":"6"}

So, as soon as Edge 17 gets released, all current JS features that we use will be supported ("last 2 versions"). 😲

@tech4him1
Copy link
Contributor Author

tech4him1 commented Jan 24, 2018

That's what I don't know - are we transpiling for node in the build or just on the fly during testing?

@erquhart We could use "node": "current" to transform only the features that need to be for each Node version.

For example, if you are building on Node 6, arrow functions won't be converted, but they will if you build on Node 0.12.

I'm not sure I'd want the build to be different than the test environment, though, any thoughts?

@tech4him1
Copy link
Contributor Author

Hmm, but then if you run a build on an old node version (v4?) it will transpile a whole bunch of stuff.

@erquhart I guess the core question is still whether we want to have the build and test environment transpiling the same things (like you have now), or different?

@erquhart
Copy link
Contributor

erquhart commented Jan 25, 2018

We could use "node": "current" to transform only the features that need to be for each Node version. Hmm, but then if you run a build on an old node version (v4?) it will transpile a whole bunch of stuff.

Yeah we don't want to depend on the build environment, I think that makes more sense for actual Node apps.

I guess the core question is still whether we want to have the build and test environment transpiling the same things (like you have now), or different?

We could maybe run production releases without Node 6 support, but all other commands (including a non-production build command) still support it. Not transpiling anything once Edge 17 drops (maybe Q2/Q3 based on recent major release dates?) is super compelling.

@tech4him1
Copy link
Contributor Author

Do you think there could be any problems having our production env different from our dev one?

@erquhart
Copy link
Contributor

erquhart commented Jan 25, 2018

Only if there are bugs in the browser implementations of the features we're not polyfilling, which is a really great point. Maybe just for running tests, then, since that's the only time Node would be executing our code.

@tech4him1
Copy link
Contributor Author

You're still having a different environment for your tests vs your build then.

@erquhart
Copy link
Contributor

The only difference is in browser implementations of those features vs the babel polyfills running for the tests, so a browser specific bug is the only thing that would slip through, and that's no different from what we're already doing (since our unit tests aren't running against multiple browsers).

@tech4him1 tech4him1 requested review from erquhart and removed request for Benaiah January 25, 2018 21:42
@erquhart erquhart merged commit ddbfae6 into master Jan 25, 2018
@erquhart erquhart deleted the fix-es6 branch January 25, 2018 21:43
tech4him1 added a commit that referenced this pull request Feb 21, 2018
tech4him1 added a commit that referenced this pull request Feb 27, 2018
tech4him1 added a commit that referenced this pull request Feb 27, 2018
tech4him1 added a commit that referenced this pull request Feb 27, 2018
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.

3 participants