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

Normalize property accessors for es6 namespaces and chained member/call expressions #17218

Closed
bworline opened this issue May 17, 2023 · 10 comments · Fixed by #17203
Closed

Normalize property accessors for es6 namespaces and chained member/call expressions #17218

bworline opened this issue May 17, 2023 · 10 comments · Fixed by #17203

Comments

@bworline
Copy link
Contributor

bworline commented May 17, 2023

Feature request

Currently webpack rewrites chained member and call expressions in dot notation.

e.g.,

/* module1.js */
export const obj1 = {};

/* module2.js */
import * as m1 from './module1';
export { m1 };

/* index.js */
import * as m2 from './module2';
const a = m2.m1.obj1["flip"].flap["flop"]();

generates

const a = _module2__WEBPACK_IMPORTED_MODULE_0__.m1.obj1.flip.flap.flop();

Some optimizing compilers such as the Google Closure Compiler ADVANCED mode and Terser with keep_quoted enabled have a problem with this rewrite. They use the quote notation as a way to say, "hands off" when it comes to minifying variable names so that external interfaces can be preserved. This leads to it seeing x["somevar"] and x.somevar as two different properties, such that x.somevar is minified and x["somevar"] is converted to x.somevar (shorter by one char but not minified). See the Closure docs for more.

What is the expected behavior?

The expression after the imported object (obj1 in this case) should remain untouched. That is,

const a = _module2__WEBPACK_IMPORTED_MODULE_0__.m1.obj1["flip"].flap["flop"]();

What is motivation or use case for adding/changing the behavior?

Get webpack 5 to generate output compatible with the Google Closure Compiler.

How should this be implemented in your opinion?

When HarmonyImportDependencyParserPlugin.js constructs HarmonyImportSpecifierDependency objects, it cannot know which parts of the member/call chain are the modules/imports and which parts are not. One option is for it to track and store all possibilities and their expression ranges, then choose which to use later in the FlagDependencyUsagePlugin when webpack has knowledge about all imports and exports.

Are you willing to work on this yourself?

Yes. I've created a failing-test PR (#17203) for now.

@TheLarkInn
Copy link
Member

@vankop would you be able to point out which specific areas should be covered to achieve this? You worked a little bit in this area.

@bworline bworline changed the title Normalize property accessors for es6 namespaces and chained member expressions Normalize property accessors for es6 namespaces and chained member/call expressions May 17, 2023
@vankop
Copy link
Member

vankop commented May 18, 2023

hm.. not sure what to suggest. It affects parser api in sensitive part, if we would like to change this. So basically what webpack does here:

lets give an example a["b"].c["d"]

For each chain member => if it is computed ["balbla"] leads to computed: true => webpack tries to evaluate member ( parser.evaluateExpression(member) => if result is BasicEvaluateExpresion ( dont remember correct class name ) =>
webpack check evaluated.asString() and if it is a string => continue loop

If all members are static analyzable by webpack ( evaluated or computed=false ) it calls callMemberChain ( dont remember correct hook name ) hook, with members list as string[] ( first of all it will try to call callExpression a.b.c.d, I guess )

callMemberChain.call("a.b.c", ["d"]);
callMemberChain.call("a.b", ["c", "d"])
callMemberChain.call(importTag, ["b", "c", "d"])

So what you need is to change hook api :

  • add 3rd param there
  • handle this isComputed thing in all members hooks in parser ( I guess it is in one place )
  • pass additional info in dependency
  • render in template in a way it was defined

Another option:

  • improve Google Closure Compiler to handle this =)

However, I have a question why it is an issue? with mangling enabled in webpack all chain members that are exports will be mangled, if member is a part of module object, like in your example, will it even work on module level? maybe it works when modules are concatenated?

@bworline
Copy link
Contributor Author

RE: why is this an issue. Mangling just mangles the imported object and not any chaining thereafter. Which is good, because it doesn't rewrite or control the other side of the code where the chained objects are created so mangling any further would break code at runtime. RE: concatenation. We can't rely on concatenation as (a) it is an option the user can choose to disable, and (b) there are plenty of cases that cause concatenation bail-out even when it is enabled.

Webpack 4 was able to work with the Closure Compiler for excellent minification. Webpack 5 has broken that, and if our team is to move to Webpack 5 then we need to get this functionality restored.

The core issue is that the chain elements following obj1 are not the responsibility of Webpack, and should not be rewritten.

Webpack doesn't even consistently rewrite elements. In the case of optional chaining, Webpack currently bails out and stops rewriting the expression at that point. My guess is that this was done because the goal was to rewrite at least the imported object reference, and it is easy to detect and use this optional point because it is guaranteed that there are no nested module or imported objects referenced in an optional way.

What I'm looking for is to have Webpack determine the actual point of the imported object in the expression and stop rewriting there so it at most rewrites the imported object reference.

I don't think that isComputed is sufficient because the module namespaces and imported objects always need to be rewritten in dot notation to match the generated export code. So we still need to know where in the chained expression the imported object stops and its first member access starts.

@vankop
Copy link
Member

vankop commented May 19, 2023

In case of optional chaining webpack always bailouts, so in example bellow:

1

export {a..d};

2.js

export * as a from "1";
export * as b from "11";
export * as c from "111";
export * as d from "1111";

3.js

export * as a from "2";
export * as b from "2";
export * as c from "222";
export * as d from "2222";

main.js

import * as D from "3";

console.log(D.b?.a.a); // this is same as import D.b for webpack

in this case tree-shaken will be only module 3 ( a,c,d exports are unused ), and everything else will be bundled as "used in unknown way"

@vankop
Copy link
Member

vankop commented May 19, 2023

What I'm looking for is to have Webpack determine the actual point of the imported object

there is no way to do this on module level, thats why webpack need to preserve all computed=true and then use it for non-exports members in chain

@vankop
Copy link
Member

vankop commented May 19, 2023

maybe there is another option when 3rd argument ( probably it is 4th since full expression always also provided ) in hook is where expression range of Ns member ended e.g.:

callMemberChain.call(importTag, currentExpression, ["b", "c", "d"], fullExpression, [10, 12, 14])

so in dependency webpack will have range ( expression range ), where range[0] will be start and one number of array in argument will be end. This way webpack can always rewrites only exports part @TheLarkInn

@bworline
Copy link
Contributor Author

Ok, so that will give all of the possible ranges. When rewriting the expression, how to best determine the exports part of an expression and pick the correct range?

@vankop
Copy link
Member

vankop commented May 21, 2023

you need to take a look in Dependency Template, I guess this is some handler in RuntimeTemplate

@bworline
Copy link
Contributor Author

bworline commented May 21, 2023

I mean, given the expression "a.b.c.d", how do I determine that a is a module, b is a namespace, and c is the export?

@bworline
Copy link
Contributor Author

bworline commented Jun 5, 2023

Thanks for your explanations and help, @vankop. I was able to figure it out from there.

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

Successfully merging a pull request may close this issue.

4 participants