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

Dynamic Imports that are marked external are hoisted #17986

Open
mohamedmansour opened this issue Jan 19, 2024 · 8 comments
Open

Dynamic Imports that are marked external are hoisted #17986

mohamedmansour opened this issue Jan 19, 2024 · 8 comments

Comments

@mohamedmansour
Copy link

mohamedmansour commented Jan 19, 2024

Given this code:

// a.js
console.log('a');

// b.js
console.log('b');

// index.js
async function onButtonClick() {
  await import('./a.js') 
  await import('./b.js')
}

Where we mark b.js as external, the bundler prints this:

  • dynamically imports "b" => console logs "b"
  • User clicks on button
  • dynamically imports "a" => console logs "a"

It should rather be:

  • User clicks on button
  • dynamically imports "a" => console logs "a"
  • dynamically imports "b" => console logs "b"

Webpack hoists all external dynamic imports to the very top, not making it dynamic import. Which defeats its purpose.

{
  mode: "production",
  externalsType: 'module'
  externals: {
    /b.js/i,
  },
  output: {
    chunkFormat: 'module',
    chunkLoading: 'import',
    module: true
  }
}

It would be great if Webpack does not wrap native ESM imports with promises as well.

Other relevant information:
"webpack": "5.89.0",
"webpack-cli": "5.1.4",
"webpack-dev-server": "4.15.1",

@TheLarkInn TheLarkInn added the bug label Jan 19, 2024
@alexander-akait
Copy link
Member

alexander-akait commented Jan 20, 2024

I want to say it is not a bug, just change externalsType: 'module' to externalsType: "import",.

I think we should add a new value for externalsType - import-module/module-import (maybe the best name) and set it by default for ES modules, i.e. keep import(...) and import ... from "module"; for externals how they were written

@alexander-akait
Copy link
Member

Also you can specify the type for an each item using https://webpack.js.org/configuration/externals/#string

@mohamedmansour
Copy link
Author

Thank you @alexander-akait , that actually solves the hoisting, but it brings that hoisting still back to the main bundle into a function. For example, index.ts dynamically imports lazy.ts, and lazy.ts dynamically imports an external named external.ts. The definition of externals is hoisted to the main bundle index.js, instead it should have been inside external.js

I don't think we need another externalsType, why would someone who specifies module would want the imports hoisted for dynamic imports? That feels wrong. If they want it to be hoisted, then define them as import "foo" instead of import("foo"). It feels more like a bug. By definition in webpack docs: webpack will output ECMAScript module syntax whenever possible

My recommendation would be to keep externalsType as module and don't transform it, just keep it in its place.

@alexander-akait
Copy link
Member

@mohamedmansour No, it is not a bug, I can't find a discussion about it, but there was already a question about it here, there are scenarios when developers want to hoist (preload big modules in the first run), some want to load dynamically (lazy evaluation), especial in diffucult applications (you can improve perf in some cases, for example some modules can take a lot of time on import but you used them in rare cases and you can't change vendor library code), that is why I said, we need the another value to allow to keep dynamic import as dynamic and static as static. The same logic works for other values of the externalsType option.

@mohamedmansour
Copy link
Author

mohamedmansour commented Jan 20, 2024

It is actually a more serious bug when we move to import it breaks the natural ESM sequence since our web app is strictly ESM (esnext, modules). So we cannot use it.

We need true browser platform ESM outputs. In the example below, chromejs is marked external. And we get the error:

Uncaught TypeError: Cannot read properties of undefined (reading 'log')

For example, chrome.js is marked external:

index.ts

import './chrome.js';
import 'app.js';

chrome.js

window.chrome = {
  'log': () => console.log('logger');
} 

app.js

window.chrome.log();

To your question, when developers want to hoist big modules, can't they just do the following which solves that scenario:

import 'big-module.js';

async function OnClick() {
  await import('small-module.js');
}

Since big-module is marked as external, it will not bundle it.

Doing some re-arrangement to assume all "dynamic" imports get hoisted to the top, breaks a core use case of dynamic imports.

@alexander-akait
Copy link
Member

alexander-akait commented Jan 20, 2024

As I said above we just need a new value - import-module, we can't keep them as is when you have import or module, because it will be breaking change and can break a lot of configurations

@webpack-bot
Copy link
Contributor

This issue had no activity for at least three months.

It's subject to automatic issue closing if there is no activity in the next 15 days.

@mohamedmansour
Copy link
Author

Bump (activity)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Priority - Medium
Development

No branches or pull requests

4 participants