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

Add allowESModules option to babel-preset-react-app #5487

Merged
merged 3 commits into from Nov 21, 2018

Conversation

@Pajn
Copy link
Contributor

commented Oct 19, 2018

Please see the discussion in #5452

@stale

This comment has been minimized.

Copy link

commented Nov 18, 2018

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Nov 18, 2018

@Pajn

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2018

Bump?

@stale stale bot removed the stale label Nov 18, 2018

@Timer
Copy link
Collaborator

left a comment

I think it'd be better to use the already-existing variable name. What do you think?

@@ -29,6 +29,7 @@ module.exports = function(api, opts, env) {
var isEnvProduction = env === 'production';
var isEnvTest = env === 'test';

var allowESModules = validateBoolOption('allowESModules', opts.allowESModules, true);

This comment has been minimized.

Copy link
@Timer

Timer Nov 18, 2018

Collaborator

Let's make this default to isEnvDevelopment || isEnvProduction and use the existing name.

Suggested change
var allowESModules = validateBoolOption('allowESModules', opts.allowESModules, true);
var useESModules = validateBoolOption('useESModules', opts.useESModules, isEnvDevelopment || isEnvProduction);
@@ -140,7 +141,7 @@ module.exports = function(api, opts, env) {
// https://babeljs.io/docs/en/babel-plugin-transform-runtime#useesmodules
// We should turn this on once the lowest version of Node LTS
// supports ES Modules.
useESModules: isEnvDevelopment || isEnvProduction,
useESModules: allowESModules ? isEnvDevelopment || isEnvProduction : false,

This comment has been minimized.

Copy link
@Timer

Timer Nov 18, 2018

Collaborator

Consequently, this becomes simpler (or use the shorthand syntax):

Suggested change
useESModules: allowESModules ? isEnvDevelopment || isEnvProduction : false,
useESModules: useESModules,

@Timer Timer added the tag: internal label Nov 18, 2018

@Timer Timer added this to the 2.1.x milestone Nov 18, 2018

@Pajn

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

Seams reasonable, updated to match requested changes.

packages/babel-preset-react-app/create.js Outdated Show resolved Hide resolved
packages/babel-preset-react-app/create.js Outdated Show resolved Hide resolved
Apply suggestions from code review
Co-Authored-By: Pajn <rasmus@eneman.eu>
@Pajn

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

I misunderstood and thought you ment only the varaible name, but that's an easy fix

@Timer

This comment has been minimized.

Copy link
Collaborator

commented Nov 20, 2018

I think this behavior is best -- if undefined, it uses old (reasonable) logic. If the user defines it, uses that value.

Does this seem good to you?

@Pajn

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

Absolutely, It's even cleaner code. 👍

@Timer
Timer approved these changes Nov 21, 2018

@Timer Timer modified the milestones: 2.1.x, 2.1.2 Nov 21, 2018

@Timer Timer merged commit a5ea56a into facebook:master Nov 21, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
hisapy added a commit to iporaitech/create-react-app that referenced this pull request Nov 22, 2018
Add allowESModules option to babel-preset-react-app (facebook#5487)
* Add allowESModules option to babel-preset-react-app

* changes after feedback

* Apply suggestions from code review

Co-Authored-By: Pajn <rasmus@eneman.eu>
dardub added a commit to OffBase/create-react-app that referenced this pull request Nov 27, 2018
Merge remote-tracking branch 'upstream/master'
* upstream/master: (210 commits)
  Support setupTests.ts (facebook#5698)
  Remove unnecessary whitespace in template HTML
  Run prettier on HTML files (facebook#5839)
  Some Grammar fixes (facebook#5858)
  Fix link to page about running tests (facebook#5883)
  fix: make typescriptformatter support 0.5 of fork checker (facebook#5879)
  Always test with the latest stable Node version on Travis (facebook#5546)
  Fix propertyDecorator test
  Upgrade babel deps
  Fix annotated var test
  Fix TypeScript decorator support (facebook#5783)
  fix: add `sideEffects: false` to react-error-overlay (facebook#5451)
  Add allowESModules option to babel-preset-react-app (facebook#5487)
  Make named-asset-import plugin work with export-as syntax (facebook#5573)
  React native repository updated in README.md (facebook#5849)
  extra polyfills must be included manually (facebook#5814)
  Rename 'getting started' link to 'docs' (facebook#5806)
  docs: Simplify installing Storybook with npx (facebook#5788)
  Don't polyfill fetch for Node -- additional files (facebook#5789)
  docs: Change Storybook install documentation (facebook#5779)
  ...

@Pajn Pajn deleted the Pajn:allow-disable-es-modules branch Nov 29, 2018

@lock lock bot locked and limited conversation to collaborators Jan 18, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.