Skip to content

Comments

fix: make EnvironmentPlugin defaultValues types less strict#18715

Merged
alexander-akait merged 3 commits intowebpack:mainfrom
102:main
Sep 3, 2024
Merged

fix: make EnvironmentPlugin defaultValues types less strict#18715
alexander-akait merged 3 commits intowebpack:mainfrom
102:main

Conversation

@102
Copy link
Contributor

@102 102 commented Aug 27, 2024 •
edited
Loading

Changes introduced in adf2a6b#diff-a150fb53afee945f23f3fe7c176007111f3ba81bb0d188af4077d4a0fc4caa37R3993 prevents updating webpack to 5.94.0 in some cases because previously EnvironmentPlugin/defaultValues could be initiated with any types (as it is being also documented), but now with types update it could only be Record<string, string>.

There are checks in code that stringify value & explicitly check if undefined was a value, so it might make sense to keep this as Record<string, any>:

webpack/lib/EnvironmentPlugin.js

Lines 40 to 59 in 94aba38

const value =
process.env[key] !== undefined
? process.env[key]
: this.defaultValues[key];
if (value === undefined) {
compiler.hooks.thisCompilation.tap("EnvironmentPlugin", compilation => {
const error = new WebpackError(
`EnvironmentPlugin - ${key} environment variable is undefined.\n\n` +
"You can pass an object with default values to suppress this warning.\n" +
"See https://webpack.js.org/plugins/environment-plugin for example."
);
error.name = "EnvVariableNotDefinedError";
compilation.errors.push(error);
});
}
definitions[`process.env.${key}`] =
value === undefined ? "undefined" : JSON.stringify(value);
(which is also a documented behavior in https://webpack.js.org/plugins/environment-plugin/)

Closes #18719

What kind of change does this PR introduce?

bugfix

Did you add tests for your changes?

I'm not sure if type changes are covered by tests

Does this PR introduce a breaking change?

this PR relaxes type requirements, so there should be no breaking changes

What needs to be documented once your changes are merged?

The behavior is already documented, so there will be no requirements for documentation changes

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 27, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@webpack-bot
Copy link
Contributor

webpack-bot commented Aug 27, 2024

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

@webpack-bot
Copy link
Contributor

webpack-bot commented Aug 28, 2024

Hi @102.

Just a little hint from a friendly bot about the best practice when submitting pull requests:

Don't submit pull request from your own main branch. It's recommended to create a feature branch for the PR.

You don't have to change it for this PR, just make sure to follow this hint the next time you submit a PR.

@alexandrevryghem alexandrevryghem mentioned this pull request Aug 30, 2024
Merged
@102 102 mentioned this pull request Sep 2, 2024
Merged
@alexander-akait alexander-akait merged commit c86ab47 into webpack:main Sep 3, 2024
@defunctzombie
Copy link
Contributor

defunctzombie commented Sep 3, 2024

Is it actually true that the value could be any though? Seems like the change to have the specific type it could be is a better direction than going back to any?

@102
Copy link
Contributor Author

102 commented Sep 3, 2024

Is it actually true that the value could be any though? Seems like the change to have the specific type it could be is a better direction than going back to any?

I think that default values are stringified with JSON.stringify and anything stricter than any could be considered breaking change, as stringified values could be of any type per https://github.com/microsoft/TypeScript/blob/main/src/lib/es5.d.ts#L1151

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexander-akait alexander-akait alexander-akait approved these changes

Assignees

No one assigned

Labels

PR: reviewed-approved

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

EnvironmentPlugin types don't allow passing null

4 participants

@102 @webpack-bot @defunctzombie @alexander-akait