Skip to content

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

Merged
Timer merged 3 commits into
react:masterfrom
Pajn:allow-disable-es-modules
Nov 21, 2018
Merged

Add allowESModules option to babel-preset-react-app#5487
Timer merged 3 commits into
react:masterfrom
Pajn:allow-disable-es-modules

Conversation

@Pajn

@PajnPajn commented Oct 19, 2018

Copy link
Copy Markdown
Contributor

Please see the discussion in #5452

@stale

staleBot commented Nov 18, 2018

Copy link
Copy Markdown

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.

@stalestaleBot added the stale label Nov 18, 2018
@Pajn

Pajn commented Nov 18, 2018

Copy link
Copy Markdown
ContributorAuthor

Bump?

@stalestaleBot removed the stale label Nov 18, 2018

@TimerTimer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

var isEnvProduction = env === 'production';
var isEnvTest = env === 'test';

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Suggested change
varallowESModules=validateBoolOption('allowESModules',opts.allowESModules,true);
varuseESModules=validateBoolOption('useESModules',opts.useESModules,isEnvDevelopment||isEnvProduction);

// We should turn this on once the lowest version of Node LTS
// supports ES Modules.
useESModules: isEnvDevelopment || isEnvProduction,
useESModules: allowESModules ? isEnvDevelopment || isEnvProduction : false,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

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

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

Pajn commented Nov 19, 2018

Copy link
Copy Markdown
ContributorAuthor

Seams reasonable, updated to match requested changes.

Comment threadpackages/babel-preset-react-app/create.js Outdated
Comment threadpackages/babel-preset-react-app/create.js Outdated
Co-Authored-By: Pajn <rasmus@eneman.eu>
@Pajn

Pajn commented Nov 20, 2018

Copy link
Copy Markdown
ContributorAuthor

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

@Timer

Copy link
Copy Markdown
Contributor

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

Pajn commented Nov 21, 2018

Copy link
Copy Markdown
ContributorAuthor

Absolutely, It's even cleaner code. 👍

@TimerTimer modified the milestones: 2.1.x, 2.1.2Nov 21, 2018
@Timer
Timer merged commit a5ea56a into react:masterNov 21, 2018
dardub added a commit to OffBase/create-react-app that referenced this pull request Nov 27, 2018
* upstream/master: (210 commits)
Support setupTests.ts (react#5698)
Remove unnecessary whitespace in template HTML
Run prettier on HTML files (react#5839)
Some Grammar fixes (react#5858)
Fix link to page about running tests (react#5883)
fix: make typescriptformatter support 0.5 of fork checker (react#5879)
Always test with the latest stable Node version on Travis (react#5546)
Fix propertyDecorator test
Upgrade babel deps
Fix annotated var test
Fix TypeScript decorator support (react#5783)
fix: add `sideEffects: false` to react-error-overlay (react#5451)
Add allowESModules option to babel-preset-react-app (react#5487)
Make named-asset-import plugin work with export-as syntax (react#5573)
React native repository updated in README.md (react#5849)
extra polyfills must be included manually (react#5814)
Rename 'getting started' link to 'docs' (react#5806)
docs: Simplify installing Storybook with npx (react#5788)
Don't polyfill fetch for Node -- additional files (react#5789)
docs: Change Storybook install documentation (react#5779)
...
@Pajn
Pajn deleted the allow-disable-es-modules branch November 29, 2018 14:57
@locklockBot locked and limited conversation to collaborators Jan 18, 2019
Sign up for freeto 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.

3 participants

@Pajn@Timer@facebook-github-bot