isAsyncChunkPushExpression is checking for left side which cannot be easily determined

[Gongreg]

Issue description

webpack-bundle-analyzer/src/parseUtils.js

Lines 191 to 214 in 757c12e

	function isAsyncChunkPushExpression(node) {
	const {
	callee,
	arguments: args
	} = node;
	return (
	callee.type === 'MemberExpression' &&
	callee.property.name === 'push' &&
	callee.object.type === 'AssignmentExpression' &&
	callee.object.left.object &&
	(
	callee.object.left.object.name === 'window' ||
	// `self` is a common output.globalObject value used to support both workers and browsers
	callee.object.left.object.name === 'self' ||
	// Webpack 4 uses `this` instead of `window`
	callee.object.left.object.type === 'ThisExpression'
	) &&
	args.length === 1 &&
	args[0].type === 'ArrayExpression' &&
	mayBeAsyncChunkArguments(args[0].elements) &&
	isModulesList(args[0].elements[1])
	);
	}

These lines

webpack-bundle-analyzer/src/parseUtils.js

Lines 201 to 208 in 757c12e

	callee.object.left.object &&
	(
	callee.object.left.object.name === 'window' ||
	// `self` is a common output.globalObject value used to support both workers and browsers
	callee.object.left.object.name === 'self' ||
	// Webpack 4 uses `this` instead of `window`
	callee.object.left.object.type === 'ThisExpression'
	) &&

Are checking if .push is called on window, self or this.
So for the simple cases:

window.webpackJsonp_someCustomName = (window.webpackJsonp_someCustomName || []).push(...)

it works well.

But sometimes Webpack creates left side output which can be MemberExpression:

(("undefined" != typeof self ? self : this).webpackJsonp_someCustomName = ("undefined" != typeof self ? self : this).webpackJsonp_someCustomName || []).push(...);

I am thinking is there any need for checking of the left side? We are already checking a lot of things and

mayBeAsyncChunkArguments(args[0].elements) && isModulesList(args[0].elements[1])

should make us confident that it is indeed a module.

[Gongreg]

We cannot check left part because it can be dynamically set with custom globalObject, chunkCallbackName & jsonpFunction

	globalObject: "(typeof self !== 'undefined' ? self : this)",

Or we could check the left side and allow MemberExpression but in that case we might as well remove the check, since it feels really redundant.

[valscion]

Thanks for opening the issue! ☺️

It's a bit hard to understand because I don't immediately get to the same level of context as you have. Would you be able to explain what kind of webpack configuration outputs the bundle you are seeing, and then we could figure out what code needs to be changed and how?

Thanks ☺️

[Gongreg]

Hello :)

Yes. In this case all you have to do is manually change
output.globalObject to a custom string inside Webpack config.

[valscion]

But sometimes Webpack creates left side output which can be MemberExpression:

(("undefined" != typeof self ? self : this).webpackJsonp_someCustomName = ("undefined" != typeof self ? self : this).webpackJsonp_someCustomName || []).push(...);

I take it that this is the crux of this issue and that this situation happens when output.globalObject is a custom string?

In that case, we'd appreciate a PR with a test case covering this, and even happier we'd be if we could have a fix, too 😄. I wrote some instructions in another issue recently on how to go about doing this kind of code change, see here: #347 (comment)

Feel free to do some code changes and then we can discuss the approaches when we have running code and CI is running the tests ☺️. It's easier that way than discussing potential code modifications without CI running the tests.

[Gongreg]

Of course I will make a PR/add test case :)

There is one additional case that is not covered by this module parser (async web worker chunks which are not using .push for modules, but I am investigating it at the moment, trying to understand when it happens). Will report/pr it when I find it.

[valscion]

Thanks! As long as there's an additional test case, and code isn't hugely refactored in the same PR as a fix is done in, it should be a no-brainer for us to get the fixes merged ☺️.

Larger refactors would be nicer if they would be done in their own PR outside of the fixes themselves, if at all possible. This way us maintainers won't have to figure out the potential impact of refactoring together with fixes to some scenarios.

So even if the fixes would make the code look even worse (adding more branches to the conditionals), I'd be grateful if such a PR was first done and then once that is merged in, a refactor PR in its own would be nice ☺️

[Gongreg]

Would you be more for adding additional MemberExpression check or removal of left side check all together? For me the left side check seems redundant, but maybe there is some case when it is needed? Couldn't find anything about it in the history of the file.

[valscion]

I don't really know why we need to be so strict in checking the left-hand side here. Maybe if all the tests pass with the left-side check being more relaxed or removed altogether is good enough?

Does @th0r have any insight on why the left-hand side expression check has been there?

[Gongreg]

For me from the history seems like it was simply added while working on chunks, and then more different scenarios were found and added on top as additional checks. So most likely left side can be skipped. I will make a pr without it.