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

Fix Webpack 4 DefinePlugin config #1286

Merged
merged 1 commit into from
Jun 28, 2018
Merged

Fix Webpack 4 DefinePlugin config #1286

merged 1 commit into from
Jun 28, 2018

Conversation

jbruni
Copy link
Contributor

@jbruni jbruni commented Jun 26, 2018

What?

I've found that the usage of webpack.DefinePlugin at Cornerstone's Webpack configuration is wrong:

new webpack.DefinePlugin({
    'process.env.NODE_ENV': 'development',
}),

According to official DefinePlugin documentation, if the definition value is a string, it will be used as a code fragment.

The plugin does a direct text replacement. The recommended way, broadly used, and also referred in official documentation, is to wrap the string in a JSON.stringify call:

new webpack.DefinePlugin({
    'process.env.NODE_ENV': JSON.stringify('development'),
}),

This way, in code, process.env.NODE_ENV will be properly substituted by the string "development", instead of the literal code snippet development, which results in a 'development' is not defined error because development is considered an undefined variable.

I've stumbled upon this issue when adding Vue.js and vue-loader as dependencies. When bundling Vue.js, Webpack substitutes process.env.NODE_ENV in Vue.js code. Due to the misconfiguration, Vue.js fails with a development is not defined error.

Tickets / Documentation

Post Scriptum

In fact, this whole DefinePlugin usage in Cornerstone config seems obsolete, now that Webpack 4 automatically uses the mode configuration value for process.env.NODE_ENV replacements.

I have completely removed the whole DefinePlugin usage from Webpack config locally, and everything worked. If we were setting other definitions, or if we wanted to override mode value in process.env.NODE_ENV, then it would make sense to keep using DefinePlugin.

@junedkazi
Copy link
Contributor

@jbruni the changes look good. Can you also add a changelog entry please.

@jbruni
Copy link
Contributor Author

jbruni commented Jun 28, 2018

@junedkazi - 👍 I've added a changelog entry.

@junedkazi junedkazi merged commit d47c637 into bigcommerce:master Jun 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants