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: don't override process variable in DefinePlugin configuration #960

Merged

Conversation

drazik
Copy link
Contributor

@drazik drazik commented Apr 6, 2021

Following the warning on https://webpack.js.org/plugins/define-plugin/:

When defining values for process prefer
'process.env.NODE_ENV': JSON.stringify('production') over
process: { env: { NODE_ENV: JSON.stringify('production') } }.
Using the latter will overwrite the process object which can break
compatibility with some modules that expect other values on the
process object to be defined.

This fixed a conflict warning I had when using dotenv-webpack plugin:

DefinePlugin
Conflicting values for 'process.env'

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @drazik!

This seems to make sense - thank you. However, there are some tests that are failing on Travis. with an error about not being able to read NODE_ENV from undefined. Can you take a look?

Thanks!

@Kocal
Copy link
Contributor

Kocal commented Apr 23, 2021

Should be this considered as a breaking change? Or at least being documented in a changelog?

At work we use the following code to inject SENTRY_DSN and SENTRY_ENVIRONMENT env vars in the bundle (from a .env file or from process.env):

Encore
  .configureDefinePlugin((options) => {
    const dotenvEnv = dotenv.config();
    if (dotenvEnv.error) {
      throw dotenvEnv.error;
    }

    const env = { ...dotenvEnv.parsed, ...process.env };

    ['SENTRY_DSN', 'SENTRY_ENVIRONMENT'].forEach((envVar) => {
      options['process.env'][envVar] = JSON.stringify(env[envVar]);
    });
  })

And I guess it won't works until we change options['process.env'][envVar] to options['process.env.' + envVar] right?
WDYT?

Thanks

@drazik drazik force-pushed the fix-define-plugin-override-process-env branch 3 times, most recently from 8598b07 to d4e1d5f Compare April 26, 2021 14:28
@drazik
Copy link
Contributor Author

drazik commented Apr 26, 2021

@weaverryan I fixed most of the test. One of them is still failing but I don't understand if it's related to my changes or not. And if it is related to my changes, what can I do about it?

@Kocal you are totally right, this is a breaking change for people that customize the DefinePlugin like you do. And you are also right about the changes it requires in your case.

@weaverryan
Copy link
Member

Due to the breaking change (good catch), I'd love to stuff this into a new major release instead of a minor.

@weaverryan weaverryan added this to the Encore 2.0 milestone Jan 21, 2022
@weaverryan weaverryan force-pushed the fix-define-plugin-override-process-env branch from d4e1d5f to fbe9393 Compare April 22, 2022 19:03
@weaverryan
Copy link
Member

Thank you @drazik!

@weaverryan weaverryan merged commit ffb7f20 into symfony:main Apr 22, 2022
weaverryan added a commit to weaverryan/webpack-encore that referenced this pull request Apr 22, 2022
weaverryan added a commit that referenced this pull request May 2, 2022
…eaverryan)

This PR was squashed before being merged into the main branch.

Discussion
----------

Allowing new major versions of outdated dependencies

Fixes #1029

~~A bit more work to do. For now, I'm focused only on the main dependencies (not the `devDependencies`).~~

~~You could argue that this will require a new major version of webpack-encore. However, there is a precedence for allowing major dependency upgrades on minor versions in the past: https://github.com/symfony/webpack-encore/releases/tag/v1.2.0. Assuming none of the libraries have underlying changes that are likely to affect users, I'd like to do that again.~~ Edit: likely we should have a new major release.

## Dependencies

* ~`chalk`~: kept at v4 because v5 requires esm. v4 is stable.
* `clean-webpack-plugin`: `3->4` No breaking changes
* `css-loader`: `5->6`: Min node to 12.3.0,  other changes: [CHANGELOG](https://github.com/webpack-contrib/css-loader/blob/master/CHANGELOG.md#-breaking-changes)
* `css-minimizer-webpack-plugin`: `2->3` Min node to 12.3.0
* `loader-utils`: REMOVED
* mini-css-extract-plugin `1.5->2.2.1` Min node to 12.13.0, Webpack to 5.0.0, other changes: [CHANGELOG](https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/CHANGELOG.md#-breaking-changes)
- ~`pkg-up`~: Keeping version `3.1` because `4` requires esm.
- `pretty-error`: `3.0->4.0` Min node to 12.0
- `resolve-url-loader` `3.0->5.0` Min node to 12.0, postcss ^8.0, remove `rework` engine,  other changes: [CHANGELOG](https://github.com/bholloway/resolve-url-loader/blob/v4-maintenance/packages/resolve-url-loader/CHANGELOG.md)
- `style-loader`: `2->3` Min node to 12.13, Min webpack to 5.0 other changes: [CHANGELOG](https://github.com/webpack-contrib/style-loader/blob/master/CHANGELOG.md#-breaking-changes)
- `yargs-parser`: `20.2->21` Min node to 12.0

## Dev Dependencies
- `eslint`: `7->8` Min node to `12.22`
- `eslint-webpack-plugin`: `2.5->3` min node to `12`. Min webpack `5`
- `fork-ts-checker-webpack-plugin`: `5->6` [CHANGELOG](https://github.com/TypeStrong/fork-ts-checker-webpack-plugin/releases/tag/v6.0.0)
- `fs-extra`: `9->10` Min node to `12`. Potentially breaking changes: [CHANGELOG](https://github.com/jprichardson/node-fs-extra/blob/master/CHANGELOG.md#breaking-changes****)
- `http-server`: `0.12->14` Min node to `12`.
- `less-loader`: `7->10`: Min node to `12`. Min webpack to `5`. `less.webpackLoaderContext` was removed, please use `pluginManager.webpackLoaderContext`
- `mocha`: `8->9`: min node to `12`. [CHANGELOG](https://github.com/mochajs/mocha/blob/master/CHANGELOG.md#boom-breaking-changes)
- `postcss-loader`: `4-5->6`: min node to `12`. min webpack to `5`.
- `preact`: `8->10`: [CHANGELOG](https://github.com/preactjs/preact/releases/tag/10.0.0)
- `sass-loader`: `9-10-11->12` min node to `12.13`. Min webpack to `5`
- `sinon`: `9->12`
- ~`strip-ansi`~: Need to keep `6` because `7` is esm only. Min node to `12`
- `stylus`: `0.54->0.56`
- `stylus-loader`: `3-4-5->6`: Min node to `12.13`. Min webpack to `5`. Other breaking changes: [CHANGELOG](https://github.com/webpack-contrib/stylus-loader/blob/master/CHANGELOG.md#400-2020-09-29)
* `vue-loader`: `16-17` No important changes: https://github.com/vuejs/vue-loader/blob/next/CHANGELOG.md#1700-2021-12-12

TODOs:

* [x] A) Check CHANGELOG of each library to see what changes, if any, we need.
* [x] B) Bump *our* CHANGELOG for changes people should watch out for
* [x] C) Get tests passing

Commits
-------

f1a1db6 Updating Changelog for #960
e8c6d25 bumping preact min to fix test
e4495aa upgrading babel to work with eslint plugin
e853d9e Bumping eslint-webpack-plugin support to fix bugs
a7d817d Writing out babel.config.js for users when using eslint plugin
a4aec57 Converting warning to error, which is consistent with babel env config method
1dc344c dropping Node 10 in CI
c4351d8 removing already unused, deprecated option
69ecce9 Removing deprecated Encore.enableEslintLoader()
993e364 allowing new major versions of outdated dependencies
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.

3 participants