Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESM self-import: different behaviors between shim and browser native modes #295

Closed
evisong opened this issue May 26, 2022 · 7 comments · Fixed by #311
Closed

ESM self-import: different behaviors between shim and browser native modes #295

evisong opened this issue May 26, 2022 · 7 comments · Fixed by #311

Comments

@evisong
Copy link

evisong commented May 26, 2022

Hi, thanks for this great project that enables us to use import-maps in production.

Recently we run into a tricky issue. When creating ESM bundles using latest Webpack 5, there's some self-import code at the bottom of entry ESM file. The bundles work well in latest Chrome, however they fail to initialize when handled in shim mode.

I've made a minimal example to reproduce this. Below is the JS part, the complete example is uploaded to https://gist.github.com/evisong/0e70d3ee5215d751e7d57d8378de9c96.

export const b = {
  m: () => console.log('Run m')
};

import { b as c } from './module-self-import.mjs';

const m = c.m;

export default function a() {
  console.log('Run a');
  m();
}

In native mode, the browser console shows:

Run inline
Run a
Run m

However in shim mode, the browser console prompts an error:

module.mjs:7 Uncaught TypeError: Cannot read properties of undefined (reading 'm')
    at module.mjs:7:13

To workaround this blocking issue, we have to disable shim mode, so we temporarily inline es module URLs instead of using elegant importmaps.

I'm not sure if this is a common use case since I've only seen it in Webpack generated bundles. Could you share some guidance? Thanks.

@guybedford
Copy link
Owner

Thanks for posting this case, it is useful to know that this is an important feature for Webpack output.

I was able to prototype an approach that can solve this issue in #296.

Unfortunately it does require some lexer work in guybedford/es-module-lexer#116 to properly get it to a point where it can be comprehensive and something we can merge.

I don't currently have much bandwidth to work on the above further, but will leave the approach open to track progress. If anyone is interested in working on or sponsoring this work further that would be a huge help too.

@evisong
Copy link
Author

evisong commented May 30, 2022

@guybedford Appreciate your quick response. For now, it seems magic to me that the 3 LOC in #296 fixes this. I'll take some time this week to dig into it, after that if I might fully understand it and am confident enough, then I'll be glad to contribute.

@tommie
Copy link
Contributor

tommie commented Jul 25, 2022

I stumbled on this issue with import map polyfill in Firefox: https://gist.github.com/tommie/f20e4c297cf86044681abd04df3d6ce5

The #296 fix works for this repro case as well. Thanks!

Same thing as @evisong, really: ESM Webpack 5 output from https://github.com/webpack/webpack/blob/8f87b50dc7ac24eb5c91fd0d55a22e34e252863c/lib/esm/ModuleChunkFormatPlugin.js#L157

(It seems this code generator changed in v5.64.4, but the output code works the same.)

What's the next step here?

IIUC, the guybedford/es-module-lexer#116 fix is most importantly to avoid requiring filter() for default? How do the other bullet points relate to #296?

@tommie
Copy link
Contributor

tommie commented Jul 25, 2022

Hmm, I spoke too soon. Webpack also does renaming of exports:

export const id = "a";
export const ids = ["a"];
export const modules = {
  // technical mumbojumbo that returns an exported A. :)
};

// load runtime
import __webpack_require__ from "./a.js"*/'blob:http://localhost/705a1176-4361-4227-962c-d32b813a020f';
var __webpack_exec__ = (moduleId) => (__webpack_require__(__webpack_require__.s = moduleId))
import * as __webpack_chunk_0__ from /*"./b.js"*/'blob:http://localhost/d302fc97-8409-4f57-8e08-9e4e21f5eeee'
;import{u$_}from'blob:http://localhost:4433/d302fc97-8409-4f57-8e08-9e4e21f5eeee';u$_({ id,ids,modules,A });  // BAD: With #296 it fails because A is not defined.

__webpack_require__.C(__webpack_chunk_0__);  // BAD: Without #296 it fails in here, because __webpack_chunk_0__ contains undefineds.
__webpack_exec__("../node_modules/webpack-dev-server/client/index.js?...");
var __webpack_exports__ = __webpack_exec__("./src/a.js");
var __webpack_exports__A = __webpack_exports__.A;
export { __webpack_exports__A as A };

This last part is generated by https://github.com/webpack/webpack/blob/main/lib/library/ModuleLibraryPlugin.js#L95.

Here's a repro for that: https://gist.github.com/tommie/7a76b4591cbad0e7b94c0d2e7f33042a

@tommie
Copy link
Contributor

tommie commented Jul 25, 2022

I have something working, based on #296:

Looks like it can load my webpack output.

@guybedford
Copy link
Owner

@tommie amazing work! We probably need to extend the handling in es-module-lexer to have a boolean property like "ambient" or something to indicate it is an ambient export and not a functional export (export { a as b } from 'y' is ambient, while import { a as b } from 'y'; export const a = b; is executed and dependent on execution ordering).

The work you've done though in es-module-lexer looks like that is the 90% and I would be happy to merge it with that change. Can you add tests and create a PR further?

@guybedford
Copy link
Owner

Also rather than explicitly tracking default, I think we may just need to track valid identifiers. I believe there is valid identifier checking logic in both the JS and Wasm builds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants