Parcel: Bundle with hoisting crashes when importing MaterialUI <Paper /> (easy to reproduce)

[Cristy94]

🐛 bug report

My app was crashing at runtime when using hoisting, I managed to find the source of the issue: it crashes when the <Paper /> component in MaterialUI is used. I assume this is an issue with parcel, as it works fine without hoisting.

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

{
    "scripts": {
        "start": "parcel src/index.html",
        "build": "parcel build src/index.html"
    },
    "devDependencies": {
        "@babel/core": "^7.9.0",
        "@babel/plugin-proposal-class-properties": "^7.8.3",
        "@babel/preset-env": "^7.9.0",
        "@babel/preset-react": "^7.9.1",
        "@babel/preset-typescript": "^7.9.0",
        "parcel": "^2.0.0-nightly.157"
    },
    "dependencies": {
        "@material-ui/core": "^4.9.7",
        "react": "^16.13.1",
        "react-dom": "^16.13.1"
    },
    "app": "dist/index.html",
    "targets": {
        "app": {
            "publicUrl": "./"
        }
    }
}

🤔 Expected Behavior

It also works when --no-scope-hoist is not used.

😯 Current Behavior

Running parcel build src/index.html generates the bundle, but crashes on runtime.

Uncaught ReferenceError: $e683e5038a7eb5723a023aa38328ec8$import$Paper is not defined

💁 Possible Solution

🔦 Context

💻 Code Sample

import React from "react";
import { render } from "react-dom";
import { Paper } from '@material-ui/core';

function Main() {
    return <Paper />;
}

render(<Main />, document.getElementById("app"));

🌍 Your Environment

| Software | Version(s) |
| ---------------- | ---------- |
| Parcel | ^2.0.0-nightly.157
| Node | 10
| npm/Yarn | 6.9
| Operating System | Windows 10

[mischnic]

Try deleting the cache, the example works for me.

[wbinnssmith]

cc @padmaia re: cache.

@Cristy94 can you provide any more context as to the steps you took to get here, or provide a set of reproduction steps to reach this again once you've cleared the cache?

[mischnic]

This likely happened because some asset is missing in the bundlegraph and causing that identifier to be missing in the imports map:
https://github.com/parcel-bundler/parcel/blob/94fd99d5db39f9c54d1daf380fd5b87f3d8e5e85/packages/shared/scope-hoisting/src/link.js#L108

I did get this error myself as well when working on the REPL, I think it happened when adding a new import but I can't reproduce it anymore.

[Cristy94]

It also works for me sometimes now if I delete the cache, and always works with --no-cache. But in my project, it still happens even though I always build with --no-cache.

I can always reproduce the error like this:

1. Delete previous cache folder.
2. Build the above example
3. Add this Theme.ts fille next to index.tsx
4. Update index.tsx to import that Theme.ts file to this, like this:

import React from "react";
import { render } from "react-dom";
import { Paper, ThemeProvider } from '@material-ui/core';
import { ustTheme } from './Theme';

function Main() {
    return <ThemeProvider theme={ustTheme}><Paper /></ThemeProvider>;
}

render(<Main />, document.getElementById("app"));

1. Run build again. It will crash at runtime. Clearing the cahce fixes it. Running the build again with same cache shows the same error.

---

I also sometimes got this error while tring to reproduce the error above, again, clearing the cache or running with --no-cache fixed it.

@parcel/packager-js: External modules are not supported when building for browser
Error: External modules are not supported when building for browser
    at Object.generateExternalImport (node_modules\@parcel\scope-hoisting\lib\formats\global.js:52:9)
    at exit (node_modules\@parcel\scope-hoisting\lib\link.js:552:36)
    at NodePath._call (node_modules\@babel\traverse\lib\path\context.js:55:20)
    at NodePath.call (node_modules\@babel\traverse\lib\path\context.js:42:17)
    at NodePath.visit (node_modules\@babel\traverse\lib\path\context.js:99:8)
    at TraversalContext.visitQueue (node_modules\@babel\traverse\lib\context.js:112:16)
    at TraversalContext.visitSingle (node_modules\@babel\traverse\lib\context.js:84:19)
    at TraversalContext.visit (node_modules\@babel\traverse\lib\context.js:140:19)
    at Function.traverse.node (node_modules\@babel\traverse\lib\index.js:84:17)
    at traverse (node_modules\@babel\traverse\lib\index.js:66:12)

[Cristy94]

Easier to reproduce with this index.tsx and the Theme.ts from above, just run once without importing it and once after importing _Theme.ts_.

import React from "react";
import { render } from "react-dom";

// 1. Run with this
import { Paper} from '@material-ui/core';
function Main() {
    return <Paper />;
}

// 2. Run with this and comment (1)
// import { Paper, ThemeProvider } from '@material-ui/core';
// import { ustTheme } from './Theme';
// function Main() {
//   return <ThemeProvider theme={ustTheme}><Paper /></ThemeProvider>;
// }

render(<Main />, document.getElementById("app"));

If you want to reproduce this multiple times you have to clear cache before running (1) again.

**1. rmdir /s .parcel-cache

1. npm run build
2. Comment (1), uncomment (2)
3. npm run build**

Let me know if you can reproduce it like this.

[mischnic]

👍

Simpler reproduction:

import React from "react";
import { render } from "react-dom";

// -- 1. Run with this
import { Paper } from "@material-ui/core";
function Main() {
    return <Paper />;
}
// --

// -- 2. Run with this and comment (1) out
// import { Paper, ThemeProvider } from "@material-ui/core";
// function Main() {
//  return (
//      <ThemeProvider>
//          <Paper />
//      </ThemeProvider>
//  );
// }
// --

render(<Main />, document.getElementById("app"));

When I ran into this, it was also when adding a specifier to an existing import.

[mischnic]

Well, even with

/* other.js */
export const foo = 1234;
export const bar = 7890;

/* index.js */
// (1)
import { foo } from "./other";
console.log(foo);

// (2)
import { foo, bar } from "./other";
console.log(foo, bar);

[mischnic]

The problem is that when the asset is changed, the JSTransformer mutates dependency.symbols
https://github.com/parcel-bundler/parcel/blob/94fd99d5db39f9c54d1daf380fd5b87f3d8e5e85/packages/shared/scope-hoisting/src/hoist.js#L499
but that is somehow not reflected in the asset graph (when adding dependency.symbols to the graphviz output, this map doesn't change).

cc @padmaia I have no idea where (in the AssetGraph(Builder)?) this is handled

[Cristy94]

Any progress on this? Can I help in any way?

[padmaia]

@mischnic When a transformation finishes, the AssetRequestRunner calls assetGraph.resolveAssetGroup() with the results here: https://github.com/parcel-bundler/parcel/blob/v2/packages/core/core/src/requests/AssetRequestRunner.js#L73. Dependency connections are established here: https://github.com/parcel-bundler/parcel/blob/v2/packages/core/core/src/AssetGraph.js#L278.

Haven't had a chance to look into, but let me know if you need more guidance with what's going in the the AssetGraphBuilder. (Also sorry for being slow to reply to my notifcations, been feeling a little overwhelmed these past few weeks)

[mischnic]

A dependency's id doesn't factor in the symbols (and even worse, the id doesn't change when mutating dep.symbols), so this
https://github.com/parcel-bundler/parcel/blob/2f6274311889cad76fbdf2dafe60fc23478476d6/packages/core/core/src/AssetGraph.js#L269
will reuses a existing dependency with the same id (which in this case has different symbols).

Should we make Dependency.symbols a $ReadOnlyMap and allow modifying that only by calling addDependency with the same modifier as we do here (in this case necessary because of isWeak):
https://github.com/parcel-bundler/parcel/blob/2f6274311889cad76fbdf2dafe60fc23478476d6/packages/shared/scope-hoisting/src/hoist.js#L593-L598
Would probably be the best solution so that we can truly rely on the dependency id on caching?