feat: support using map as the externals root by fi3ework · Pull Request #18784 · webpack/webpack

fi3ework · 2024-09-24T05:24:20Z

What kind of change does this PR introduce?

Related discussion: #16272.

Support fine-grained control of external types using an external category in a map format.

While the current external function type can achieve this to somehow, it has a major drawback: the string and regex checks must be overridden in the function type to perform these judgments.

The external type of map is added here to prevent the ongoing spread of subsequent types like 'module-import' We can imagine that the 'module-import' type no longer exists. Instead, we use the new external type map, which we can implement easily as follows.

externals: {
  module: 'module',
  import: 'import',
}

In the future, when there are more similar requirements of 'A-B', we can use map to solve it.

Overview changes:

add map form of externals type, and divide it in right before creating ExternalModule
add the category to external function params

Did you add tests for your changes?

Yes.

Does this PR introduce a breaking change?

I believe not, 'externalsType' just adds a new non-string type.

What needs to be documented once your changes are merged?

Yes, if this could be merged.

webpack-bot · 2024-09-24T05:24:25Z

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)

lib/ExternalModuleFactoryPlugin.js

alexander-akait · 2024-09-26T12:13:36Z

lib/ExternalModuleFactoryPlugin.js

+				? esmType
+				: /** @type {ExternalsCategory} */ (dependencyType);
+
+		const resolved = externalType[externalCategory] || externalType.fallback;


Should we have a default value for fallback?

I think it's hard to decide for user which type should be used as fallback, and it might introduce more implicit settings.

alexander-akait · 2024-09-27T11:46:31Z

test/configCases/externals/object-externals-root/webpack.config.js

+	externalsType: {
+		"static-import": "commonjs",
+		"dynamic-import": "import",
+		fallback: "window"


Can we add a test case with amd, also we support system and other

To be honestly we can use https://github.com/webpack/webpack/blob/main/lib/dependencies/CommonJsFullRequireDependency.js#L93 and https://github.com/webpack/webpack/blob/main/lib/dependencies/ImportDependency.js#L40

Do I understand correctly that we are trying to provide the opportunity to use one or another type external depending by dependency category?

Maybe we can introduce new function to our dependencies

Can we add a test case with amd, also we support system and other

Sure.

Do I understand correctly that we are trying to provide the opportunity to use one or another type external depending by dependency category?

The original requirement is that when we build for NPM package, we need to keep import() as import() and convert import to commonjs (similar to #16272). Considering that we previously added the type of 'module-import', supporting map types for externalsTypes would be more scalable and more convenient for users. We had thought about exposing some fields of dependency in a function before, but this would expose internal abstractions, so we need to categorize possible categories for users in advance. The finer the category judgment here, the better it can prevent us from having to split existing categories in the future.

Maybe we can introduce new functions into our dependencies.

I need to review all possible external types again, I previously thought that category covered all enumerations, but now we need to consider the field type.

Can we add a test case with amd, also we support system and other

I added a test case for amd. But I am not clear of the system here, does Systemjs got a dedicated import method?

alexander-akait · 2024-09-27T11:47:39Z

lib/ExternalModuleFactoryPlugin.js

+				? "static-import"
+				: dependency instanceof ImportDependency
+					? "dynamic-import"
+					: undefined;


Ideally in future we should refactor and move it to dependency property/method, I can refactor and show you

IIUC, move to a new field, and the field will represent for all available keys in externals type map?

Yeah, because we can have external url's (CSS for examle), external new URL(...), external @import's (yeah, I don't think developers often using it, but will be great to solve it in universal way), that is why I think we need getExternalCategory or something like that

I added a new getExternalCategory to ModuleDependency, and some of the subclass implemented it. As of now:

AMDRequireItemDependency: "amd"

CommonJsRequireDependency: "commonjs"

HarmonyImportDependency: "static-import"

ImportDependency: "dynamic-import"

Please take a look, and I think there are more dependencies to add but I'm not sure what they are. Any suggestion please.

And the name of "static-import" and "dynamic-import" looks a bit weird for me as it hasn't been present in webpack before, maybe import and import() are better as mentioned above.

fi3ework · 2024-10-28T18:27:45Z

Friendly ping @alexander-akait, could you please review the comments so we can improve the PR one more step. Thanks! 😊

fi3ework · 2024-11-06T15:04:42Z

@alexander-akait Friendly ping 😊. If this PR changes too many scopes and may not be merged, we could consider adding a new commonjs-import external type. It reserves import() while changing other external requests to require. And won't change too much code.

The most essential demand is to reserve dynamic import for CJS as it's supported since 13.2.0. Dynamic import could allow user to import an ESM module, whereas require can't. I think this is commonly used in Node.js targeted scenes.

esbuild support emit import() if target is above node 13.2.0 and could disable this behavior by --supported:dynamic-import=false.

webpack-bot added the PR: unreviewed label Sep 24, 2024

alexander-akait reviewed Sep 25, 2024

View reviewed changes

lib/ExternalModuleFactoryPlugin.js Show resolved Hide resolved

fi3ework force-pushed the externalsType-obj branch from a07d3c8 to b578a20 Compare September 25, 2024 18:13

alexander-akait reviewed Sep 26, 2024

View reviewed changes

alexander-akait reviewed Sep 27, 2024

View reviewed changes

webpack-bot added the PR: conflict label Oct 2, 2024

fi3ework force-pushed the externalsType-obj branch from df41278 to ceccc39 Compare October 17, 2024 12:00

webpack-bot removed the PR: conflict label Oct 17, 2024

fi3ework force-pushed the externalsType-obj branch from ceccc39 to 35828a7 Compare October 27, 2024 17:48

feat: support using map as the externals root

6ae4cfe

fi3ework force-pushed the externalsType-obj branch from 35828a7 to 6ae4cfe Compare October 28, 2024 16:57

Uh oh!

Conversation

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

Uh oh!

Uh oh!

webpack-bot commented Sep 24, 2024

Uh oh!

Uh oh!

alexander-akait Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

fi3ework Sep 26, 2024

Choose a reason for hiding this comment

Uh oh!

alexander-akait Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

fi3ework Sep 29, 2024 • 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 Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

alexander-akait Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

fi3ework Sep 29, 2024

Choose a reason for hiding this comment

Uh oh!

alexander-akait Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

fi3ework Oct 17, 2024 • 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 Oct 28, 2024

Uh oh!

fi3ework commented Nov 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fi3ework commented Sep 24, 2024 •

edited

Loading

fi3ework Sep 29, 2024 •

edited

Loading

fi3ework Oct 17, 2024 •

edited

Loading