External async dynamic imports aren't replaced correctly

[MartinDawson]

🐛 bug report

I'm using yarn workspaces.

Ran parcel build

source:

async exportWorkbook() {
    const { writeFile } = await import('xlsx');

    console.log(writeFile)
  }

output:

  async exportWorkbook() {
        const { writeFile: writeFile  } = await $ab6d51fd7c132abf$importAsync$95ba5fe6ab928e6a;
         
        console.log(writeFile)
    }

Error: ReferenceError $ab6d51fd7c132abf$importAsync$95ba5fe6ab928e6a is not defined

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

package.json:

  "source": "src/index.ts",
  "exports": "./dist/index.modern.js",
  "main": "./dist/index.js",
  "module": "./dist/index.module.js",
  "types": "dist/index.d.ts",
  "devDependencies": {
    "@parcel/transformer-sass": "2.0.0-nightly.907",
    "@parcel/transformer-typescript-types": "2.0.0-nightly.907",
    "@parcel/core": "2.0.0-nightly.905",
    "parcel": "2.0.0-nightly.905",
  }

Root level package.json:

 "devDependencies": {
    "@parcel/core": "2.0.0-nightly.905",
    "@parcel/packager-ts": "2.0.0-nightly.907",
    "@parcel/transformer-typescript-types": "2.0.0-nightly.907",
}

{
  "your": { "config": "here" }
}

🤔 Expected Behavior

Should not throw an error.

😯 Current Behavior

Throws an error due to not compiling the dynamic import correctly.

index.module.js:5228 Uncaught (in promise) ReferenceError: $ab6d51fd7c132abf$importAsync$95ba5fe6ab928e6a is not defined
    at $ab6d51fd7c132abf$var$Exporter.exportWorkbook (index.module.js:5228)
    at $372c619c9f53bf45$var$Toolbar.setValue (index.module.js:4694)
    at HTMLButtonElement.eval (index.module.js:4328)
exportWorkbook @ index.module.js:5228
setValue @ index.module.js:4694
eval @ index.module.js:4328

🌍 Your Environment

Software	Version(s)
Parcel	2.0.0-nightly
Node	12.20.1
npm/Yarn	1.19.2
Operating System	Ubuntu 12.x

[amiller-gh]

This appears to be broken in 2.0.1 as well.

For me, it only occurs with non-relative module imports.

  @main static async quit() {
    const { app } = await import('electron');
    app.quit();
  }

converts to

static async quit() {
   const { app  } = await require("1c2e8670fdf805c8");
   app.quit();
}

and fails with 02:21:08.836 › Error: Cannot find module '1c2e8670fdf805c8'

Config is:

{
  "context": "electron-main",
  "source": "./src/main.ts",
  "distDir": "./dist/src",
  "outputFormat": "commonjs",
  "isLibrary": false,
  "scopeHoist": false,
  "optimize": false
}

However, fails similarly will all combinations of isLibrary, scopeHoist, and optimize set to true.

[amiller-gh]

@mischnic this is blocking adoption for us – really want to get this build time improvement in! (~2 minutes down to ~5s. Absolutely stellar). If I can be of any help to land a fix here, feel free to point me at the general location of the offending code / test suite and we'll try to take a stab at it this weekend.

[mischnic]

Tests regarding building CommonJS and ESM libraries are here: https://github.com/parcel-bundler/parcel/blob/v2/packages/core/integration-tests/test/output-formats.js

The problematic code is somewhere in here probably. But the scope hoisting packager is rather complex, so good luck

[amiller-gh]

Status update: I've confirmed that the import symbols for async imported external modules are not present in the bundle graph when buildReplacements is called:

https://github.com/parcel-bundler/parcel/blob/e0440dcbe6d6e419a412c8c568d94715cdb4282a/packages/packagers/js/src/ScopeHoistingPackager.js#L540

This means that when buildAsset encounters the import symbol it can not find the appropriate replacement and falls back to the import symbol:

https://github.com/parcel-bundler/parcel/blob/e0440dcbe6d6e419a412c8c568d94715cdb4282a/packages/packagers/js/src/ScopeHoistingPackager.js#L480

[amiller-gh]

Update 2: When the DefaultBundler encounters an external dependency node of type esm and lazy, it appears unable to resolve any assets:

https://github.com/parcel-bundler/parcel/blob/9176b984bb69fafab1ad9a8b0c8a5742360d3f33/packages/bundlers/default/src/DefaultBundler.js#L80-L85

Logging at this point for external lazy esm deps will produce:

  console.log(dependency.id, assets.length, !!resolution)
  // Logs: <id> 0 false

But for relative lazy esm dependencies it will produce:

  console.log(dependency.id, assets.length, !!resolution)
  // Logs: <id> 1 true

Continuing down the rabbit hole

[amiller-gh]

Side note as a small performance thing:

bundleGraph.getDependencyAssets(dependency) is called right before bundleGraph.getResolvedAsset(dependency) in DefaultBundler.js, but getDependencyAssets is actually called inside of getResolvedAsset, resulting in duplicate traversal work!

https://github.com/parcel-bundler/parcel/blob/v2/packages/bundlers/default/src/DefaultBundler.js#L78-L79

[amiller-gh]

Hmkay, I've hit my limit tonight, but I think these assets never seem to hit bundleGraph.createAssetReference, so it never gets replaced.

The NodeResolver does properly fetch the asset object from the AST, but somehow never gets registered with the bundle graph. I don't have enough context yet on the connective tissue between the two to know why.

I could also be talking out of my ass, so please let me know if it seems like I'm off track here!

[amiller-gh]

Hey @mischnic – have a draft PR up for the feature here: https://github.com/parcel-bundler/parcel/pull/7384

Would love a quick preliminary look to validate that I've made the changes in the appropriate places. Will push up integration tests once I get the not from you. I've already validated that it works in our current code base.

@devongovett, somewhat tangentially: We build a relatively large Electron app and I have an idea for a backwards compatible, low-touch feature addition that will dramatically improve integration (and maybe even fix one of the most frustrating Electron development pain points).

Given my new context with the ScopeHoistingPackager from this little PR, I think I could add it relatively quickly. Mind if I open an RFC issue for the enhancement to the electron-renderer context target I've been toying with?

[devongovett]

Thanks for the PR! Will take a look soon. An RFC sounds great!

[RobbieMcKinstry]

Ah, I recently came across this bug myself. Has anyone found a workaround? I'd like to continue to use await import() for lazy-loading where possible.