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

Treat sideEffects array as inclusion list #227

Merged
merged 4 commits into from Jun 22, 2019

Conversation

mikeharder
Copy link
Contributor

@@ -1,6 +1,7 @@
{
"main": "./index.js",
"sideEffects": [
"./index.js",
Copy link
Member

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 🙄

Copy link
Contributor Author

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?

src/index.js Outdated Show resolved Hide resolved
Copy link
Member

@lukastaegert lukastaegert left a 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

@mikeharder
Copy link
Contributor Author

@lukastaegert: I believe I have made all the requested changes, with the exception of removing ./index.js from the sideEffects array, since I'm not sure if the resulting behavior is a bug or by design.

Copy link
Member

@lukastaegert lukastaegert left a 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!

@lukastaegert lukastaegert merged commit d49e257 into rollup:master Jun 22, 2019
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 this pull request may close these issues.

sideEffects array is treated as exclusion list
2 participants