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.

Sign up for GitHub

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

Jump to bottom

Switches to use babel-preset-env #13

Merged
merged 1 commit into from
Jun 16, 2017
Merged

Switches to use babel-preset-env #13

merged 1 commit into from
Jun 16, 2017

Conversation

goatslacker
Copy link
Collaborator

Babel7 is set to deprecate the ES2015 preset and the new recommended
approach is to use babel-preset-env which also lets you fine tune the
transforms specific to the environments the code will be run on. This
commit moves our implementation to use babel-preset-env as well as lists
a few browsers we support.

Fixes #10

I tested this by checking in files that were transpiled with master's configuration and then switching over to babel-preset-env and re-running the transpile step. git diff was empty.

goatslacker
goatslacker commented Jun 15, 2017
View reviewed changes
index.js Outdated
modules: false,
targets: {
browsers: [
'Chrome >= 35',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are the minimum versions of browsers we support globally based on The Formula™

We can also add iOS, Android, and Edge here. Anything else?

This was referenced Jun 15, 2017
Closed
Closed
@goatslacker goatslacker force-pushed the babel-preset-env branch 2 times, most recently from f2b7971 to c0af967 Compare June 15, 2017 21:53
ljharb
ljharb reviewed Jun 16, 2017
View reviewed changes
index.js
return {
presets: [
preset,
require('babel-preset-env').default(null, {
debug: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's "debug" do? (#14 (comment))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/babel/babel-preset-env#debug

console.logs the plugins being used and the browser versions that are being applied to each plugin.

Copy link
Collaborator

Choose a reason for hiding this comment

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

should it be set to process.env.NODE_ENV !== 'production'?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, there's currently no harm in always keeping debug turned on. Since this only happens at build time nothing will be logged in production.

Copy link
Collaborator

Choose a reason for hiding this comment

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

gotcha, sounds good

Copy link
Member

Choose a reason for hiding this comment

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

I've also seen this show up when running other things, like jest. I think we should not enable this by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to expose an option for this... the below is currently being logged 118 times in our build, it's pretty noisy.

Modules transform: false

Using plugins:
  check-es2015-constants {"android":"30","chrome":"35","explorer":"9","safari":"8","ucandroid":"1"}
  transform-es2015-arrow-functions {"android":"30","chrome":"35","explorer":"9","safari":"8","ucandroid":"1"}
  transform-es2015-block-scoped-functions {"android":"30","chrome":"35","explorer":"9","safari":"8","ucandroid":"1"}
  transform-es2015-block-scoping {"android":"30","chrome":"35","explorer":"9","safari":"8","ucandroid":"1"}
  transform-es2015-classes {"android":"30","chrome":"35","explorer":"9","safari":"8","ucandroid":"1"}
  transform-es2015-computed-properties {"android":"30","chrome":"35","explorer":"9","ucandroid":"1"}
  transform-es2015-destructuring {"android":"30","chrome":"35","edge":"14","explorer":"9","firefox":"52","safari":"8","ucandroid":"1"}
  transform-es2015-duplicate-keys {"android":"30","chrome":"35","explorer":"9","safari":"8","ucandroid":"1"}
  transform-es2015-for-of {"android":"30","chrome":"35","edge":"14","explorer":"9","firefox":"52","safari":"8","ucandroid":"1"}
  transform-es2015-function-name {"android":"30","chrome":"35","edge":"14","explorer":"9","firefox":"52","safari":"8","ucandroid":"1"}
  transform-es2015-literals {"android":"30","chrome":"35","explorer":"9","firefox":"52","safari":"8","ucandroid":"1"}
  transform-es2015-object-super {"android":"30","chrome":"35","explorer":"9","safari":"8","ucandroid":"1"}
  transform-es2015-parameters {"android":"30","chrome":"35","explorer":"9","firefox":"52","safari":"8","ucandroid":"1"}
  transform-es2015-shorthand-properties {"android":"30","chrome":"35","explorer":"9","safari":"8","ucandroid":"1"}
  transform-es2015-spread {"android":"30","chrome":"35","explorer":"9","safari":"8","ucandroid":"1"}
  transform-es2015-sticky-regex {"android":"30","chrome":"35","explorer":"9","safari":"8","ucandroid":"1"}
  transform-es2015-typeof-symbol {"chrome":"35","explorer":"9","safari":"8","ucandroid":"1"}
  transform-es2015-unicode-regex {"android":"30","chrome":"35","explorer":"9","safari":"8","ucandroid":"1"}
  transform-exponentiation-operator {"android":"30","chrome":"35","explorer":"9","safari":"8","ucandroid":"1"}
  syntax-trailing-function-commas {"android":"30","chrome":"35","explorer":"9","safari":"8","ucandroid":"1"}
babel-preset-env: `DEBUG` option

Using targets:
{
  "android": "30",
  "chrome": "35",
  "edge": "14",
  "explorer": "9",
  "firefox": "52",
  "safari": "8",
  "ucandroid": "1"
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Totally agree; I think we should change it to be off by default and use an env var

index.js
require('babel-plugin-transform-es3-member-expression-literals'),
require('babel-plugin-transform-es3-property-literals'),
require('babel-plugin-transform-jscript'),
require('babel-plugin-transform-exponentiation-operator'),
require('babel-plugin-syntax-trailing-function-commas'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

just triple-checking that env covers the above two? (#14 (comment))

@goatslacker goatslacker force-pushed the babel-preset-env branch 2 times, most recently from 11f4448 to b00e03a Compare June 16, 2017 20:37
ljharb
ljharb reviewed Jun 16, 2017
View reviewed changes
index.js Outdated
};

function buildTargets(options) {
return Object.assign(defaultTargets, options.additionalTargets);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this mutates the defaults; i think we want Object.assign({}, defaultTargets, options.additionalTargets) here (or return { ...defaultTargets, ...options.additionalTargets };)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be OK since this config is loaded once. But yeah I'll amend.

index.js Outdated
} else {
preset = require('babel-preset-es2015-without-strict');
}
const targets = options && options.targets
Copy link
Collaborator

Choose a reason for hiding this comment

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

options && options.targets || buildTargets(options) should work here

@goatslacker
Switches to use babel-preset-env
a3b73f6 
Babel7 is set to deprecate the ES2015 preset and the new recommended
approach is to use babel-preset-env which also lets you fine tune the
transforms specific to the environments the code will be run on. This
commit moves our implementation to use babel-preset-env as well as lists
a few browsers we support.
goatslacker
goatslacker commented Jun 16, 2017
View reviewed changes
index.js
} else {
preset = require('babel-preset-es2015-without-strict');
}
const targets = (options && options.targets) || buildTargets(options);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ljharb heads up, lint complained about the use of && and || so I wrapped it in parens.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, good point :-)

@goatslacker goatslacker merged commit 98c9c1d into master Jun 16, 2017
@goatslacker goatslacker deleted the babel-preset-env branch June 16, 2017 23:38
@goatslacker
Copy link
Collaborator Author

It's a major change but it's not shouldn't be a breaking change: 2.3.0 or 3.0.0?

@ljharb
Copy link
Collaborator

ljharb commented Jun 16, 2017

It's not breaking, so I think 2.3.0

@fvgs fvgs mentioned this pull request Jun 27, 2017
Closed
@lencioni lencioni mentioned this pull request Jun 6, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

@gdborton gdborton gdborton left review comments

@lencioni lencioni Awaiting requested review from lencioni

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@goatslacker @ljharb @loganfsmyth @lencioni @gdborton