Add support for variations of sideEffects value
#6314
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`false` - marks each module as side-effect free `"false"` - assumes `false`, except for files named "false" [...] - compares file name to globs
|
That's funny. If I make the changes beautify-lint wants, ESLint is upset. This may have to wait until tomorrow. |
reergymerej
commented
Jan 14, 2018
| sideEffects !== false; | ||
| }; | ||
| const evaluateWithFalseValue = (sideEffects, file) => | ||
| (sideEffects === "false") ? |
There was a problem hiding this comment.
This is ugly and doesn't quite match the other code, but it was the only thing I could find to make both the linters happy.
ooflorent
reviewed
Jan 14, 2018
schemas/WebpackOptions.json
Outdated
| "items": { | ||
| "description": "Globs for child modules with side effects", | ||
| "minLength": 1, | ||
| "type": "string" |
There was a problem hiding this comment.
What about regexps? Since ou are converting globs into regexps, supporting them would be trivial.
There was a problem hiding this comment.
I can add that. There was also the suggestion to support an object, but I wasn’t sure how far to go. I’ll add support for more variations.
There was a problem hiding this comment.
7d8700f adds support for all the variants I've run across in the issues. I need help defining these in the schema.
|
Feel free to update eslintrc to prevent beautify failures. |
Boolean(-ish)
"sideEffects": true
"sideEffects": false
"sideEffects": "true"
"sideEffects": "false"
File Path
"sideEffects": "./foo.js"
Glob
"sideEffects": "./foo/**/*.js"
Regular Expression
"sideEffects": "foo.+\\.js$"
Map of File Paths
"sideEffects": {
"./clean.js": false,
"./dirty.js": true
}
Array of Options
"sideEffects": [...] // any of the options above
#6065 (comment)
#6074 (comment)
#6074 (comment)
sokra
requested changes
Jan 16, 2018
schemas/WebpackOptions.json
Outdated
| @@ -930,7 +930,19 @@ | |||
| }, | |||
| "sideEffects": { | |||
| "description": "Flags a module as with or without side effects", | |||
| "type": "boolean" | |||
| "anyOf": [ | |||
There was a problem hiding this comment.
You should not change the schema at all. This is not the schema for the package.json, but for the configuration. You are not changing the behavior of sideEffects in the module.rules.
There was a problem hiding this comment.
Thanks for the clarification.
| @@ -117,5 +116,34 @@ class SideEffectsFlagPlugin { | |||
| }); | |||
| }); | |||
| } | |||
|
|
|||
| static moduleHasSideEffects(modulePath, flagValue) { | |||
There was a problem hiding this comment.
This is too much. Please only support boolean, string as glob and array of strings as globs.
There was a problem hiding this comment.
I agree it is too much. I was trying to satisfy all the variations in the related issues that came up. I'll trim it down.
| return flagValue; | ||
| case "string": | ||
| { | ||
| if(flagValue === "false" || flagValue === "true") { |
There was a problem hiding this comment.
It's kind of weird to support wrong booleans "false". Remove this.
There was a problem hiding this comment.
Supporting "false" in #6074 was the genesis of this PR. Should we drop support for it?
|
|
||
| let matchesRegex; | ||
| try { | ||
| matchesRegex = (new RegExp(flagValue, "i")).test(modulePath); |
| case "object": | ||
| return Array.isArray(flagValue) ? | ||
| flagValue.some(value => SideEffectsFlagPlugin.moduleHasSideEffects(modulePath, value)) : | ||
| Object.keys(flagValue).some(key => key === modulePath && |
There was a problem hiding this comment.
Remove it. Only arrays are supported. Other objects should be ignored for now.
| SideEffectsFlagPlugin.moduleHasSideEffects(modulePath, flagValue[key]) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
I try to keep it as simple as possible, because this increases the change of other bundles adding support for this too.
|
|
||
| it("should understand a glob", () => { | ||
| SideEffectsFlagPlugin.moduleHasSideEffects("./src/x/y/z.js", "./src/**/*.js").should.eql(true); | ||
| SideEffectsFlagPlugin.moduleHasSideEffects("./x.js", "./src/**/*.js").should.eql(false); |
There was a problem hiding this comment.
Partial paths should be supported too
SideEffectsFlagPlugin.moduleHasSideEffects("./src/x/y/z.js", "*.js").should.eql(true);
SideEffectsFlagPlugin.moduleHasSideEffects("./src/x/y/z.js", "src").should.eql(true);
SideEffectsFlagPlugin.moduleHasSideEffects("./src/x/y/z.js", "src/**/z.js").should.eql(true);
SideEffectsFlagPlugin.moduleHasSideEffects("./src/x/y/z.js", "x/**/z.js").should.eql(true);
|
@reergymerej Thanks for your update. I labeled the Pull Request so reviewers will review it again. @sokra Please review the new changes. |
|
I'm not sure why, but a bunch of tests seemingly unrelated to my changes are failing. I'm trying to figure it out now. FWIW, the integration and unit tests for |
|
The CI failure is really weird. I try to rerun the CI. |
|
@reergymerej Please review the following output log for errors: See complete report here. |
|
hmm something in your PR seem to cause all these errors. Maybe in the new dependency? |
|
Sorry for not catching this before the PR. I've tried peeling the layers back to see if I can find it to no avail. I'm going trash this branch, redo it from scratch, and see if I can spot the problem straight away. |
|
As it turns out, the change to the plugin was showing up in other tests, causing them to blow up in bizarre ways. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What kind of change does this PR introduce?
This enhances an existing feature, identifying modules with side effects.
Did you add tests for your changes?
Yes.
If relevant, link to documentation update:
I updated the schemas/WebpackOptions.json. It just dawned on me that the docs are in a different repo. I will create a PR there with the changes as needed.
Summary
Import rewrite optimizations depend on child modules possibly introducing side effects. Authors can declare which child modules have side effects. This change allows users more granular control when specifying which modules are impure. See #6074 for more details.
Does this PR introduce a breaking change?
Nope.
Other information
This is my first real PR for this project, so I likely goofed something. Please don't hold back on the criticism.