feat: add new external type "module-import" by fi3ework · Pull Request #18620 · webpack/webpack

fi3ework · 2024-07-29T17:26:57Z

What kind of change does this PR introduce?

Resolve #17986.

Documentation update: webpack/webpack.js.org#7345.

Following content maybe somehow out of context. This PR's final implementation is to resolve #17986.

A new external type has been introduced, mainly for the purpose of output clean artifact during bundling library. The name of this "module-import" is referenced from #17986 (comment), but I'm not sure if the current implementation fully meets the intention of this comment. This PR is an incomplete implementation, only implementing the handling of external processing for dynamic imports.

Here's what it does:

Given following config:

// webpack.config.js
externals: {
  path: 'path node-commonjs',
  fs: 'module-import fs',
  'node:fs': 'module-import node:fs',
  'node-fs': 'module-import fs',
},

source

it("should allow async externals", async () => {
	const fs1 = await import("fs");
	const fs2 = await import("node:fs");
	const fs3 = await import("node-fs");

	expect(fs1).toStrictEqual(fs2);
	expect(fs1).toStrictEqual(fs3);
});

output

var __webpack_exports__ = {};
/*!******************!*\
  !*** ./index.js ***!
  \******************/
it("should allow async externals", async () => {
	const fs1 = await import("fs");
	const fs2 = await import("node:fs");
	const fs3 = await import("fs");

	expect(fs1).toStrictEqual(fs2);
	expect(fs1).toStrictEqual(fs3);
});

There is a slightly hacky bit here, where we're deleting ImportDependency at finishModules stage to prevent any runtime code from being generated. But I couldn't find a better way to prevent ImportDependencyTemplate from modifying the runtime code. Possibly by introducing a new DependencyLibraryTemplate specifically for library patterns could achieve that. Do you have any suggestions on how to suppress runtime code generation? @alexander-akait

Did you add tests for your changes?

Yes.

Does this PR introduce a breaking change?

No.

What needs to be documented once your changes are merged?

The PR is still in draft mode, it should add update the document before merged.

webpack-bot · 2024-07-29T17:27:04Z

For maintainers only:

This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
This needs to be backported to webpack 4 (issue will be created when merged)

alexander-akait

I think we can preserve import ... from '...' and import() using instanceof without extra hooks like here https://github.com/webpack/webpack/blob/main/lib/ExternalModuleFactoryPlugin.js#L120

fi3ework · 2024-07-30T14:27:05Z

Thanks for the suggestion. IIUC, does the code snippet demonstrate whether it meets your suggestions? Or keep the original method that removing the dep from module block.

// lib/ExternalModuleFactoryPlugin.js

+					if (
+						type === "module-import" &&
+						dependency instanceof ImportDependency
+					) {
+						dependency.runTemplate = false;
+					}

					callback(
						null,
						new ExternalModule(
							externalConfig,
							type || globalType,
							dependency.request,
							dependencyMeta
						)
					);

// lib/dependencies/ImportDependency.js

+		if (dep.run_template === false) {
+			return;
+		}

		const content = runtimeTemplate.moduleNamespacePromise({
			chunkGraph,
			block: block,
			module: /** @type {Module} */ (moduleGraph.getModule(dep)),
			request: dep.request,
			strict: /** @type {BuildMeta} */ (module.buildMeta).strictHarmonyModule,
			message: "import()",
			runtimeRequirements
		});
		source.replace(dep.range[0], dep.range[1] - 1, content);
	}

alexander-akait · 2024-07-30T15:03:12Z

I don't think we need ExternalModuleImportDependency at all, just check if it is ImportDependency or HarmonyImportDependency and so use import() or import ... from '...' (just set it in dependencyMeta) when we use module-import

fi3ework · 2024-07-30T15:24:28Z

just check if it is ImportDependency or HarmonyImportDependency and so use import() or import ... from '...' (just set it in dependencyMeta) when we use module-import

IIUC, ImportDependencyTemplate will replace import('xxx') to Promise.resolve(/* import() */).then(__webpack_require__.bind(__webpack_require__, "xxx")) (https://github.com/webpack/webpack/blob/main/lib/dependencies/ImportDependency.js#L125-L135). How could we stop the source replacement through dependencyMeta? 🤔

I don't think we need ExternalModuleImportDependency at all

Based on above, if we stop ImportDependencyTemplate from replacing the code. Using ExternalModuleImportDependency could achieve alias mapping (e.g. mapping import fs from 'fs' to 'import fs from 'fs-xxx') like other external.

alexander-akait · 2024-07-30T15:27:51Z

IIUC, ImportDependencyTemplate will replace import('xxx') to Promise.resolve(/* import() */).then(webpack_require.bind(webpack_require, "xxx")) (https://github.com/webpack/webpack/blob/main/lib/dependencies/ImportDependency.js#L125-L135). How could we stop the source replacement through dependencyMeta? 🤔

Hm, intresting, it seemed to me that we do nothing in this case if the module is external, can you check it?

fi3ework · 2024-07-30T15:44:45Z

Not sure what should I check, I created a online demo to show how "import" and "module" external type be processed now. 👀 hopes it might help. https://stackblitz.com/edit/github-ztep8o-umy9wa?file=dist%2Fmain.js&terminal=

The methods to deal with the ImportDependencyTemplate I know now:

In this PR: remove the ImportDependency in ExternalsPlugin, but this may affect unknown features.
The methods thought of in the above discussion: add a template flag to ImportDependencyTemplate from ExternalModuleFactoryPlugin.

fi3ework · 2024-08-01T17:52:18Z

Is there any other information I can provide to help now? 👀

alexander-akait · 2024-08-02T18:37:18Z

@fi3ework What about this? Now we keep import ... from '...'; and import('...') logic as they were written (the original issue about it), anyway if you want to keep await import('...') without extra wrapping we should make it under library: "modern-module" (and use hooks to remove/change modules there)

fi3ework · 2024-08-04T17:08:56Z

Thanks for the kind response and code update. I wrongly mixed the mission of "module-import" with "modern-module" before.

module-import external type (proposed at #17986): just a mixing of existing module and import, use module for static import and import for dynamic import, determined by the type of dependency as described above.

As for wiping out the extra runtime for modern-module library type, I'll finish that in separated PRs. module-import will be a good first step for it.

I'll update this PR later.

fi3ework · 2024-08-05T09:58:31Z

PR updated and documentation update alongside (webpack/webpack.js.org#7345).

alexander-akait

Looks good, I think we can use this value as default (https://github.com/webpack/webpack/blob/main/lib/config/defaults.js#L271) when ES modules enabled, because modules are experemental we can change the value, I think this is more semantically correct and also reduces loading time avoiding initial loading, what do you think?

alexander-akait · 2024-08-05T14:43:15Z

Also looks like we need to fix one our test with with { ... } syntax 😄

fi3ework · 2024-08-05T15:19:31Z

Looks good, I think we can use this value as default (main/lib/config/defaults.js#L271) when ES modules enabled, because modules are experemental we can change the value, I think this is more semantically correct and also reduces loading time avoiding initial loading, what do you think?

I changed the default value. Agreed that the import() should keep dynamic in semantics.

Also looks like we need to fix one our test with with { ... } syntax 😄

Fixed 😄.

alexander-akait · 2024-08-06T03:08:41Z

Let's update snapshot 😄

webpack-bot · 2024-08-06T13:03:24Z

I've created an issue to document this in webpack/webpack.js.org.

alexander-akait · 2024-08-06T13:03:29Z

Thank you

fi3ework · 2024-08-08T15:10:50Z

@alexander-akait We implement "module-import", but the premise is that we're using import or import() for external request, However, for example, if we use require('...'), "module-import" will failed to know which externals type to adopt ("import", "module", or even "node-commonjs"), and it will crash.

The possible options we may want to fall back to:

node-commonjs: to let require('path') code could run in Node.js + ESM (this is widely used).
import: maybe for some specific reason.
module: maybe for some specific reason.
commonjs: level the require there, to be bundled by a bundler.

Here are some possible ways to adding the fallback:

Work with externalsPresets.
- when externalsPresets is a node.js-related presets, fallback to node-commonjs;
- when externalsPresets === 'web', fallback to module;
- when externalsPresets === 'web-async', fallback to import;
If externalsPresets is not specified, we could set the fallback to commonjs (🤔 I'm not sure, this might break module-import's semantics)
Adding module-import-node-commonjs, module-import-import, module-import-module, module-import-commonjs, maybe too verbose.
Adding a new config exteralsFallback, but this will only work with "module-import", but adding an option on the top is a high tradeoff.

Personally, I would choose Plan 1.

What do you think, I could make a quick fix before next version releasing if the fallback is confirmed.

alexander-akait · 2024-08-08T16:59:24Z

@fi3ework hm, yeah, I see it, let's try to use 1

webpack-bot added the PR: unreviewed label Jul 29, 2024

alexander-akait reviewed Jul 30, 2024

View reviewed changes

fi3ework mentioned this pull request Jul 31, 2024

Bundler support track web-infra-dev/rslib#45

Open

42 tasks

webpack-bot added PR: conflict and removed PR: conflict labels Aug 2, 2024

alexander-akait force-pushed the new-external branch from d046b17 to bf479bb Compare August 2, 2024 18:56

fi3ework and others added 4 commits August 5, 2024 17:03

feat: add new external type "module-import"

ce24bb0

refactor: simplify logic

406ec4f

refactor

1d07f1f

refactor: clean code

f22a965

fi3ework force-pushed the new-external branch from 5c81a63 to f22a965 Compare August 5, 2024 09:06

rebase

c783012

fi3ework marked this pull request as ready for review August 5, 2024 09:54

fi3ework mentioned this pull request Aug 5, 2024

docs(externals): add import and module-import external type webpack/webpack.js.org#7345

Merged

4 tasks

fi3ework requested a review from alexander-akait August 5, 2024 13:48

alexander-akait reviewed Aug 5, 2024

View reviewed changes

cr update

93aff0c

udpate snapshot

ebc61a1

alexander-akait approved these changes Aug 6, 2024

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: unreviewed labels Aug 6, 2024

alexander-akait merged commit 740ec9e into webpack:main Aug 6, 2024

webpack-bot mentioned this pull request Aug 6, 2024

Document webpack change: feat: add new external type "module-import" webpack/webpack.js.org#7348

Closed

fi3ework mentioned this pull request Aug 7, 2024

feat: port "module-import" external type web-infra-dev/rspack#7479

Merged

2 tasks

fi3ework deleted the new-external branch August 8, 2024 15:07

This was referenced Aug 11, 2024

fix: module-import get fallback from externalsPresets #18660

Merged

fix: "module-import" simply falls back to "module" when bails web-infra-dev/rspack#7585

Merged

Uh oh!

Conversation

fi3ework commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webpack-bot commented Jul 29, 2024 • edited by alexander-akait Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexander-akait left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fi3ework commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexander-akait commented Jul 30, 2024

Uh oh!

fi3ework commented Jul 30, 2024

Uh oh!

alexander-akait commented Jul 30, 2024

Uh oh!

fi3ework commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fi3ework commented Aug 1, 2024

Uh oh!

alexander-akait commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fi3ework commented Aug 4, 2024

Uh oh!

fi3ework commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexander-akait left a comment

Choose a reason for hiding this comment

Uh oh!

alexander-akait commented Aug 5, 2024

Uh oh!

fi3ework commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexander-akait commented Aug 6, 2024

Uh oh!

webpack-bot commented Aug 6, 2024

Uh oh!

alexander-akait commented Aug 6, 2024

Uh oh!

fi3ework commented Aug 8, 2024

Uh oh!

alexander-akait commented Aug 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fi3ework commented Jul 29, 2024 •

edited

Loading

webpack-bot commented Jul 29, 2024 •

edited by alexander-akait

Loading

alexander-akait left a comment •

edited

Loading

fi3ework commented Jul 30, 2024 •

edited

Loading

fi3ework commented Jul 30, 2024 •

edited

Loading

alexander-akait commented Aug 2, 2024 •

edited

Loading

fi3ework commented Aug 5, 2024 •

edited

Loading

fi3ework commented Aug 5, 2024 •

edited

Loading