-
Notifications
You must be signed in to change notification settings - Fork 86
Treat sideEffects array as inclusion list #227
Conversation
mikeharder
commented
Jun 20, 2019
- Fixes sideEffects array is treated as exclusion list #226
@@ -1,6 +1,7 @@ | |||
{ | |||
"main": "./index.js", | |||
"sideEffects": [ | |||
"./index.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you including the index? I think it is very instructive to see that in case a side-effect-free file reexports from side-effect-ful file, only the latter file is included.
Otherwise thanks so much for spotting this. It goes to show that testing does not mean everything is working as intended if the test is bananas 🙄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remove ./index.js
from the sideEffects
array, then none of the array-
files are included:
AssertionError [ERR_ASSERTION]: Input A expected to strictly deep-equal input B:
+ expected - actual
[
'false-dep1',
'true-dep1',
'true-dep2',
- 'true-index'
+ 'true-index',
+ 'array-dep1',
+ 'array-dep3',
+ 'array-dep5',
+ 'array-index'
]
Is this by design, or might this be another bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comments regarding inclusion of index in the test and slightly simplifying the hasModuleSideEffects function
@lukastaegert: I believe I have made all the requested changes, with the exception of removing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, index needs to be included for the other files to be included because that is how side-effects work. Thanks for the fix, looks good to me!