Tree shaking and React

[rafaelrinaldi]

🐛 bug report

I am experiencing a faulty behavior when generating a production bundle using the experimental tree shaking feature.

I have already logged this in the Spectrum channel but decided to open a proper bug report as well.

🎛 Configuration (.babelrc, package.json, cli command)

I put together a repository so you can reproduce the same behavior I am experiencing: https://github.com/rafaelrinaldi/parcel-tree-shaking-jsx-error

🤔 Expected Behavior

Tree shaking should work just fine.

😯 Current Behavior

Tree shaking kinda works but seems to bypass React, logging "React is not defined" in the console.

💁 Possible Solution

I believe that this could be two of the following:

1. "React" is imported whenever JSX is used but not necessarily is being referenced, so the tree shaking might be skipping it for that reason (even though the babel-plugin-transform-react-jsx plugin added by the babel-preset-react should be replacing JSX nodes by React.createElement() calls, therefore notifying the tree shaking algorithm that the module is being used)
2. It might be an module load ordering/evaluation issue in Parcel. Perhaps even solved by Deterministic asset ids #1694

🔦 Context

I'm trying to get the experimental tree shaking feature to work in a project that uses a route-based code splitting strategy using react router and react loadable.

💻 Code Sample

I put together a repository so you can reproduce the same behavior I am experiencing: https://github.com/rafaelrinaldi/parcel-tree-shaking-jsx-error

🌍 Your Environment

Software	Version(s)
Parcel	1.9.4
Node	v10.6.0
npm	6.1.0
Operating System	macOS High Sierra

[buronnie]

Reason of the issue:
It seems that when JSX is transformed (calling babel.transformFromAst(asset.ast, asset.contents, config)), the scope binding is not updated (only AST is updated). So in the hoist phase, we fail to find binding of React because there is still JSX syntax in the scope.

Possible solution:
We need to recrawl the scope in order to get the update bindings. Maybe we can call it when we enter the hoist?

Program: {
    enter(path, asset) {
      asset.cacheData.imports = asset.cacheData.imports || Object.create(null);
      asset.cacheData.exports = asset.cacheData.exports || Object.create(null);
      asset.cacheData.wildcards = asset.cacheData.wildcards || [];
      asset.cacheData.sideEffects = asset._package && asset._package.sideEffects;
      path.scope.crawl();

@devongovett @fathyb Do you have any thoughts? 😄

[rafaelrinaldi]

@buronnie Thanks for chiming in. Yes, that possibility ran through my mind today as well. The order that the transpilation steps happen will mislead the tree shaking algo in some cases.

[rafaelrinaldi]

I've updated the test repository to use Parcel v1.9.5. Now I get a different error:

Error (from the browser console)

Uncaught TypeError: ($Mgd$init(...) , $Mgd$exports) is not a function
    at $iqAm$init (parcel-test.7f7f4a64.js:942)
    at $DGX$init (parcel-test.7f7f4a64.js:1153)
    at parcel-test.7f7f4a64.js:1332
    at parcel-test.7f7f4a64.js:42
    at parcel-test.7f7f4a64.js:45
$iqAm$init @ parcel-test.7f7f4a64.js:942
$DGX$init @ parcel-test.7f7f4a64.js:1153
(anonymous) @ parcel-test.7f7f4a64.js:1332
(anonymous) @ parcel-test.7f7f4a64.js:42
(anonymous) @ parcel-test.7f7f4a64.js:45

Breakpoint from the bundle file

// ASSET: node_modules/core-js/modules/_set-to-string-tag.js
var $iqAm$exports,
    $iqAm$var$def,
    $iqAm$var$has,
    $iqAm$var$TAG,
    $iqAm$executed = false;

function $iqAm$init() {
  if ($iqAm$executed) return;
  $iqAm$executed = true;
  $iqAm$exports = {};
  $iqAm$var$def = ($wv$init(), $wv$exports).f;
  $iqAm$var$has = ($nW2g$init(), $nW2g$exports);
  $iqAm$var$TAG = ($Mgd$init(), $Mgd$exports)('toStringTag');

  $iqAm$exports = function (it, tag, stat) {
    if (it && !$iqAm$var$has(it = stat ? it : it.prototype, $iqAm$var$TAG)) $iqAm$var$def(it, $iqAm$var$TAG, {
      configurable: true,
      value: tag
    });
  };
}

Full bundle file

[fathyb]

@rafaelrinaldi #1735 and #1737 should fix the ($Mgd$init(...) , $Mgd$exports) is not a function error. If you use it you'll need to remove "sideEffects": false from react-router-dom, we still have some issues on the sideEffects support.

[buronnie]

@devongovett I just tried release 1.9.7 which includes the fix in #1737 . The issue ($Mgd$init(...) , $Mgd$exports) is not a function is already fixed, but "React is not defined" in the console issue is still existing 🤔

[rafaelrinaldi]

@fathyb I can confirm the export issue was fixed by v.1.9.7.
@buronnie Same thing happened to me.

Updated the repo to show the React issue still persists.

[mischnic]

The React import error now has a PR #2292.

@rafaelrinaldi Now your example throws this: Error: Minified React error #130; visit https://reactjs.org/docs/error-decoder.html?invariant=130&args[]=undefined&args[]= for the full message or use the non-minified dev environment for full errors and additional helpful warnings.