Skip to content

Correct Babel dependency behavior - #5046

Merged
Timer merged 2 commits into
react:nextfrom
Timer:hotfix/correct-babel-dependency-behavior
Sep 20, 2018
Merged

Correct Babel dependency behavior#5046
Timer merged 2 commits into
react:nextfrom
Timer:hotfix/correct-babel-dependency-behavior

Conversation

@Timer

@Timer Timer commented Sep 20, 2018

Copy link
Copy Markdown
Contributor

Babel was compiling async/generators incorrectly in dependency using just preset-env. We need to rewrite these to use the non-global regenerator and apply other tweaks from the application configuration.

On a side note, it took me forever to figure out there was some bad actor in Yarn Workspaces/Babel that required me to git clean -fdx && yarn install every time I changed dependencies.js. Troubleshooting this took ages because of that.

@Timer Timer added this to the 2.0.0 milestone Sep 20, 2018
@Timer
Timer merged commit a1a08db into react:next Sep 20, 2018
@Timer
Timer deleted the hotfix/correct-babel-dependency-behavior branch September 20, 2018 20:24
@lixiaoyan

lixiaoyan commented Sep 21, 2018

Copy link
Copy Markdown
Contributor

@babel/plugin-transform-runtime will try to insert import statement by default ("module" is the default value of the sourceType config).

Before Babel:

async function fn() {}
module.exports = fn;

After Babel:

import _regeneratorRuntime from "@babel/runtime/regenerator";
import _asyncToGenerator from "@babel/runtime/helpers/asyncToGenerator";
// ...
module.exports = fn;

Mixing commonjs modules and es modules will cause webpack to generate invalid code.

@Timer

Timer commented Sep 21, 2018

Copy link
Copy Markdown
Contributor Author

Hmm, thank you for the information @lixiaoyan.
Do you have proposed better way of handling this?

@lixiaoyan

lixiaoyan commented Sep 21, 2018

Copy link
Copy Markdown
Contributor

@Timer Set sourceType to "unambiguous" for node_modules may resolve this issue but I haven't tried it yet.

@lixiaoyan

Copy link
Copy Markdown
Contributor

@Timer

Timer commented Sep 21, 2018

Copy link
Copy Markdown
Contributor Author

Awesome @lixiaoyan. Mind sending a PR changing this or do you not care for the git blame?

@Timer

Timer commented Sep 21, 2018

Copy link
Copy Markdown
Contributor Author

Even prepped the diff for you:

diff --git a/packages/react-scripts/config/webpack.config.dev.js b/packages/react-scripts/config/webpack.config.dev.js
index 12b8f8cf..0775b62a 100644
--- a/packages/react-scripts/config/webpack.config.dev.js
+++ b/packages/react-scripts/config/webpack.config.dev.js
@@ -277,6 +277,11 @@ module.exports = {
                   ],
                   cacheDirectory: true,
                   highlightCode: true,
+                  // Babel assumes ES Modules, which isn't safe until CommonJS
+                  // dies. This changes the behavior to assume CommonJS unless
+                  // an `import` or `export` is present in the file.
+                  // https://github.com/webpack/webpack/issues/4039#issuecomment-419284940
+                  sourceType: 'unambiguous',
                 },
               },
             ],
diff --git a/packages/react-scripts/config/webpack.config.prod.js b/packages/react-scripts/config/webpack.config.prod.js
index 1e0fadbf..6c44af5c 100644
--- a/packages/react-scripts/config/webpack.config.prod.js
+++ b/packages/react-scripts/config/webpack.config.prod.js
@@ -306,6 +306,11 @@ module.exports = {
                   ],
                   cacheDirectory: true,
                   highlightCode: true,
+                  // Babel assumes ES Modules, which isn't safe until CommonJS
+                  // dies. This changes the behavior to assume CommonJS unless
+                  // an `import` or `export` is present in the file.
+                  // https://github.com/webpack/webpack/issues/4039#issuecomment-419284940
+                  sourceType: 'unambiguous',
                 },
               },
             ],

@lixiaoyan

Copy link
Copy Markdown
Contributor

I think the best place to add this option is "babel-preset-react-app/dependencies.js" (it just works).
#5052

@Timer

Timer commented Sep 21, 2018

Copy link
Copy Markdown
Contributor Author

Makes sense. PR merged. 🎉 Thank you!

@lock lock Bot locked and limited conversation to collaborators Jan 19, 2019
Sign up for free to 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