Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

sideEffects array is treated as exclusion list #226

Closed
mikeharder opened this issue Jun 20, 2019 · 0 comments · Fixed by #227
Closed

sideEffects array is treated as exclusion list #226

mikeharder opened this issue Jun 20, 2019 · 0 comments · Fixed by #227

Comments

@mikeharder
Copy link
Contributor

mikeharder commented Jun 20, 2019

  • Node-Resolve Plugin Version: 5.0.3
  • Rollup Version: 1.15.6
  • Operating System (or Browser): Windows 10
  • Node Version: 10.16.0

How Do We Reproduce?

I believe the sideEffects array in package.json is being treated as an exclusion list instead of an inclusion list. The current code sets hasModuleSideEffects to true if the id does not match the filter specified by the array:

packageInfo.hasModuleSideEffects = id => !filter(id);

However, I believe this is backwards, and the correct code should be:

packageInfo.hasModuleSideEffects = id => filter(id);

The pull request which added this feature (#219) includes tests for the sideEffects array, but I believe the expected test results are backwards as well. The test package.json specifies the following filter:

"sideEffects": [
"./dep1.js",
"**/*-free.js"
]

Which should mean that dep1.js and *-free.js do have side effects, and all other files do not have side effects. However, the test asserts exactly the opposite:

'array-dep2',
'array-dep4',

Expected Behavior

Files matching the sideEffects array should have hasModuleSideEffects set to a function which returns true.

Actual Behavior

Files matching the sideEffects array have hasModuleSideEffects set to a function which returns false.

@mikeharder mikeharder changed the title sideEffects array is processed incorrectly sideEffects array is treated as exclusion list instead of inclusion list Jun 20, 2019
@mikeharder mikeharder changed the title sideEffects array is treated as exclusion list instead of inclusion list sideEffects array is treated as exclusion list Jun 20, 2019
mikeharder added a commit to mikeharder/rollup-plugin-node-resolve that referenced this issue Jun 20, 2019
lukastaegert pushed a commit that referenced this issue Jun 22, 2019
* Treat sideEffects array as inclusion list
- Fixes #226

* Rename 'true-index' to 'array-index' for clarity

* Inline unnecessary local variable

* Rename test files from "-free" to "-effect"
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant