search

rollup / rollup-plugin-node-resolve

This module has moved and is now available at @rollup/plugin-node-resolve / https://github.com/rollup/plugins/blob/master/packages/node-resolve
MIT License
468 stars 96 forks source link

sideEffects array is treated as exclusion list #226

Closed mikeharder closed 5 years ago

mikeharder commented 5 years ago

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:

https://github.com/rollup/rollup-plugin-node-resolve/blob/fe7ea0ede81f5caf08db61c6e6b6356ea7ac3ac2/src/index.js#L149

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

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

The pull request which added this feature (https://github.com/rollup/rollup-plugin-node-resolve/pull/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:

https://github.com/rollup/rollup-plugin-node-resolve/blob/fe7ea0ede81f5caf08db61c6e6b6356ea7ac3ac2/test/node_modules/side-effects-array/package.json#L3-L6

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:

https://github.com/rollup/rollup-plugin-node-resolve/blob/fe7ea0ede81f5caf08db61c6e6b6356ea7ac3ac2/test/test.js#L875-L876

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.