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

DefinePlugin regression #8293

Closed
FranckFreiburger opened this issue Oct 28, 2018 · 10 comments
Closed

DefinePlugin regression #8293

FranckFreiburger opened this issue Oct 28, 2018 · 10 comments

Comments

@FranckFreiburger
Copy link
Contributor

@FranckFreiburger FranckFreiburger commented Oct 28, 2018

Bug report

What is the current behavior?

DefinePlugin does not replace things in all situations.
I encounter this issue while upgrading from webpack v3.6.0 to v4.23.1

If the current behavior is a bug, please provide the steps to reproduce.

new webpack.DefinePlugin({
	NAME: 'foobar'
})

then:

import(`./${NAME}.yaml`)

expands to:

__webpack_require__("../mytest lazy recursive ^\\.\\/.*\\.yaml$")('./' + NAME + '.yaml')

that is not correct.

Other relevant information:
webpack version: 4.23.1
Node.js version: 10.10
Operating System: win7
Additional tools:

@ljqx
Copy link
Member

@ljqx ljqx commented Oct 28, 2018

I got some initial investigation:
ImportParserPlugin stops walk on its param when creating context, but it shouldn't.

Changing the return value to false should fix it.

I can try to send out a fix for it.

@sokra
Copy link
Member

@sokra sokra commented Oct 29, 2018

 new webpack.DefinePlugin({
-	NAME: 'foobar'
+	NAME: '"foobar"'
 })
@sokra
Copy link
Member

@sokra sokra commented Oct 29, 2018

hmm seems like to intentionally passed an expression. Than it seems to be a bug.

As workaround:

const name = NAME;
import(`./${name}.yaml`)
@FranckFreiburger
Copy link
Contributor Author

@FranckFreiburger FranckFreiburger commented Oct 29, 2018

Sorry for my ambiguous test, it should be:

new webpack.DefinePlugin({
	NAME: '"foobar"'
})

then:

var tmp = '';
import(`./${NAME}${tmp}.yaml`)

expands to:

var tmp = '';
__webpack_require__("../myTest lazy recursive ^\\.\\/foobar.*\\.yaml$")(`./${NAME}${tmp}.yaml`)
@sokra
Copy link
Member

@sokra sokra commented Oct 29, 2018

ah ok. That seem to be different.

@FranckFreiburger
Copy link
Contributor Author

@FranckFreiburger FranckFreiburger commented Nov 2, 2018

Yes, but in both situations, something is not properly expanded.

@sokra sokra closed this in #8294 Nov 3, 2018
@FranckFreiburger
Copy link
Contributor Author

@FranckFreiburger FranckFreiburger commented Nov 5, 2018

@sokra, it seems that the problem is still present (in v4.25.0)
NAME is not replaced by "foobar" as expected:

__webpack_require__("../myTest lazy recursive ^\\.\\/foobar.*\\.yaml$")('./' + NAME + tmp + '.yaml')
FranckFreiburger added a commit to FranckFreiburger/webpack that referenced this issue Nov 5, 2018
@ljqx
Copy link
Member

@ljqx ljqx commented Nov 5, 2018

It works at my side, check https://github.com/ljqx/webpack-test-import-parser.

The fix only works for template string, it doesn't include fix for string concated by +. That needs another fix to work.

I can have a try.

sokra added a commit that referenced this issue Nov 5, 2018
walk inner expressions in wrapped context
add tracking of inner expressions in wrapped BasicEvaluatedExpression
fixes #8293
closes ##8337
@sokra
Copy link
Member

@sokra sokra commented Nov 5, 2018

I already have a fix. Sorry didn't read your comment.

@sokra
Copy link
Member

@sokra sokra commented Nov 5, 2018

sokra added a commit that referenced this issue Nov 5, 2018
Bugfix for #8293
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants