[ImportParserPlugin] fix return value when creating context for `import()` #8294

ljqx · 2018-10-28T16:25:32Z

This PR fixes #8293 . Now ImportParserPlugin returns true when creating context, which stops parser walking on param of import(), then DefinePlugin fails to change value of it.

What kind of change does this PR introduce?
a bugfix

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?
nothing

…instead of true

webpack-bot · 2018-10-28T16:25:35Z

For maintainers only:

This need to be documented (issue in webpack/webpack.js.org will be filed when merged)

sokra · 2018-10-29T06:49:18Z

lib/dependencies/ImportParserPlugin.js

@@ -254,7 +254,7 @@ class ImportParserPlugin {
 				dep.loc = expr.loc;
 				dep.optional = !!parser.scope.inTry;
 				parser.state.current.addDependency(dep);
-				return true;
+				return false;


I think walking the full expression is not correct as it would handle constant pieces twice.

like this one: import(`./${CONSTANT}/${DEFINED_EXPRESSION}`)

with DefinePlugin({ CONSTANT: JSON.stringify("folder"), DEFINED_EXPRESSION: "foobar" })

Could you add this to the test?

Sadly you are right. Seems only reliable way to fix this is to walk necessary expressions in ContextDependencyHelpers. I'll push a new iteration

webpack-bot · 2018-10-29T16:34:47Z

@ljqx Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

sokra · 2018-10-30T07:44:40Z

lib/dependencies/AMDDefineDependencyParserPlugin.js

 		if (param.isConditional()) {
-			param.options.forEach(param => {
-				const result = this.processItem(parser, expr, param);
+			param.options.forEach((param, idx) => {


We need some tests for this.

I think this fails if multiple ?: operators are nested. In this case options has more than 2 elements:

import(a ? (b ? "x" : `./${CONST_PREFIX}/${DEFINED_EXPRESSION}/${CONST_SUFFIX}`) : (c ? `./${CONST_PREFIX}/${DEFINED_EXPRESSION}/${CONST_SUFFIX}` : "y") )

I think it make sense to add the expression tracking to the BasicEvaluatedExpression (i. e. setExpression for the whole thing, and setTemplateString(quasis, parts) for template strings), instead of reimplementing it here.

You can call setExpression in Parser.evaluateExpression to do it once for all

You need to change getSimplifiedTemplateResult in Parser a bit to get all the expressions too.

BasicEvaluatedExpression.setTemplateString now takes quasis which is still the same, parts which are all the quasis and epxressions as BasicEvaluatedExpressions, so all the parts of the template string.

Thanks! I've thought it's too heavy for this PR, but surely it's more appropriate for easily tracking back the expr from BasicEvaluatedExpression

sokra · 2018-10-30T07:45:21Z

lib/dependencies/ContextDependencyHelpers.js

@@ -45,7 +45,9 @@ ContextDependencyHelpers.create = (
 	Dep,
 	range,
 	param,
+	paramExpr,


You no longer need this argument, as param now contains this information.

sokra · 2018-10-30T07:48:37Z

lib/dependencies/ContextDependencyHelpers.js

@@ -84,7 +86,17 @@ ContextDependencyHelpers.create = (
 		);
 		dep.loc = expr.loc;
 		const replaces = [];
-		if (prefixRange && prefix !== prefixRaw) {
+		const templateLiteral =


You can walk all parts of the param.

If it's a constant string: replace the range with the string (push to replaces)

If it's not constant: walk it with the parser

sokra · 2018-10-30T07:49:32Z

lib/dependencies/RequireResolveDependencyParserPlugin.js

@@ -21,20 +21,24 @@ class RequireResolveDependencyParserPlugin {
 			if (expr.arguments.length !== 1) return;
 			const param = parser.evaluateExpression(expr.arguments[0]);
 			if (param.isConditional()) {
-				for (const option of param.options) {
+				param.options.forEach((option, idx) => {


All these changes can be reverted as the expression is now tracked in the BasicEvaluatedExpression.

sokra · 2018-10-30T07:52:14Z

lib/dependencies/ContextDependencyHelpers.js

 	expr,
+	parser,


To avoid a breaking change, make this argument optional. In this case expressions are not walked.

OK, thanks! Then in this PR I'll only handle import() case, then later working on AMD define, require, CommonJS require and require.resolve

sokra · 2018-10-30T07:54:31Z

lib/dependencies/ContextDependencyHelpers.js

+				parser.walkExpression(e);
+			}
+		});
+		if (prefix !== prefixRaw) {
 			replaces.push({
 				range: prefixRange,
 				value: prefix


Lower in this file, at the else case, you need to walk the whole expression.

ljqx · 2018-10-30T11:47:16Z

@sokra , I've published a new iteration. It's more readable now. Thanks!

webpack-bot · 2018-10-30T17:48:30Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

fix bug which keeps const conditional expression in bundle remove parsing code from ContextDependencyHelpers

webpack-bot · 2018-11-03T22:28:10Z

It looks like this Pull Request doesn't include enough test cases (based on Code Coverage analysis of the PR diff).

A PR need to be covered by tests if you add a new feature (we want to make sure that your feature is working) or if you fix a bug (we want to make sure that we don't run into a regression in future).

@ljqx Please check if this is appliable to your PR and if you can add more test cases.

Read the test readme for details how to write test cases.

sokra · 2018-11-03T22:28:27Z

Thanks

ljqx added 2 commits October 29, 2018 00:19

[ImportParserPlugin] when need creating context, should return false …

c5a7feb

…instead of true

add tests

1d78d91

webpack-bot added the PR: unreviewed label Oct 28, 2018

ljqx changed the title ~~import parser plugin fix context return~~ [ImportParserPlugin] fix return value when creating context for import() Oct 28, 2018

webpack-bot added the PR: CI-ok label Oct 28, 2018

sokra requested changes Oct 29, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Oct 29, 2018

ljqx added 2 commits October 30, 2018 00:33

only walk expressions that are not replaced by ContextDependency

d3041bc

enhance tests

0cbb36f

webpack-bot added PR: review-outdated PR: tests-needed and removed PR: reviewed-changes-requested labels Oct 29, 2018

sokra requested changes Oct 30, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: review-outdated labels Oct 30, 2018

ljqx added 4 commits October 30, 2018 16:22

handle conditional evaluated to template string

16ff800

add tests

84f92ab

fix after comments

25af83f

fix tests

ff3bfa1

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Oct 30, 2018

webpack-bot added PR: CI-not-ok and removed PR: CI-ok labels Oct 30, 2018

ljqx added 2 commits October 30, 2018 23:43

fix jsdoc

c740380

fix jsdoc

88aab1e

webpack-bot added the PR: CI-ok label Oct 30, 2018

webpack-bot removed the PR: CI-not-ok label Oct 30, 2018

webpack-bot removed the PR: tests-needed label Oct 30, 2018

sokra added 2 commits November 3, 2018 11:16

merge parts too when simplifying

f0dfc45

fix bug which keeps const conditional expression in bundle remove parsing code from ContextDependencyHelpers

Perform replacmenents even without parser

1a541e1

webpack-bot added the PR: tests-needed label Nov 3, 2018

fix shortcut condition

22aee1e

sokra approved these changes Nov 3, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Nov 3, 2018

sokra merged commit eb68316 into webpack:master Nov 3, 2018

sokra added a commit that referenced this pull request Nov 5, 2018

Partial update of #8294, compat layer is no longer needed

8653569

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ImportParserPlugin] fix return value when creating context for `import()` #8294

[ImportParserPlugin] fix return value when creating context for `import()` #8294

ljqx commented Oct 28, 2018

webpack-bot commented Oct 28, 2018

sokra Oct 29, 2018

ljqx Oct 29, 2018

webpack-bot commented Oct 29, 2018

sokra Oct 30, 2018

ljqx Oct 30, 2018

sokra Oct 30, 2018

sokra Oct 30, 2018

sokra Oct 30, 2018

sokra Oct 30, 2018

ljqx Oct 30, 2018

sokra Oct 30, 2018

ljqx commented Oct 30, 2018

webpack-bot commented Oct 30, 2018

webpack-bot commented Nov 3, 2018

sokra commented Nov 3, 2018

[ImportParserPlugin] fix return value when creating context for import() #8294

[ImportParserPlugin] fix return value when creating context for import() #8294

Conversation

ljqx commented Oct 28, 2018

webpack-bot commented Oct 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webpack-bot commented Oct 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljqx commented Oct 30, 2018

webpack-bot commented Oct 30, 2018

webpack-bot commented Nov 3, 2018

sokra commented Nov 3, 2018

[ImportParserPlugin] fix return value when creating context for `import()` #8294

[ImportParserPlugin] fix return value when creating context for `import()` #8294