fkhadra / react-toastify

React notification made easy 🚀 !
https://fkhadra.github.io/react-toastify/introduction
MIT License
12.58k stars 692 forks source link

Bug | Source code is wrongly eliminated during tree-shaking #641

Closed andreifloricel closed 3 years ago

andreifloricel commented 3 years ago

Do you want to request a feature or report a bug? Bug

What is the current behavior? Parts of react-toastify sources are wrongly treated as dead-code and eliminated during tree-shaking.

Ex: the imported eventManager has side-effects and it should NOT be eliminated. But this happens because currently only CSS files are marked as having side-effects: https://github.com/fkhadra/react-toastify/blob/v8.0.2/package.json#L20

What is the expected behavior?

A simple&quick solution would be to mark the library as having side-effects (in package.json): "sideEffects":true,

fkhadra commented 3 years ago

Hey @andreifloricel, is it possible for you to share the output of the bundle or the part that shows that the eventManager is eliminated? I'm trying to understand why this is happening

andreifloricel commented 3 years ago

Hi @fkhadra , unfortunately I can't provide you the bundle because it's part of a closed-source app. But I'll do my best to explain the scenario:

We develop library A which depends on a component B which depends on react-toastify.

If our library A is integrated in an app X which has the following compression configuration: (webpack.config.js)

module.exports = {
  optimization: {
    minimize: true,
    minimizer: [new TerserPlugin({
      minify:true,
      terserOptions:{
        compress:{
          side_effects: true,
          sequences: true
        }
      }
    })],
  },
};

the lines 205-229 from toast.tsx are eliminated from the production bundle of app X. If both _sideeffects and sequences options are disabled, then the previously referenced code block is retained and everything works as expected. This behaviour is somewhat explainable because the eventManager import has side-effects :)

A possible solution would be to adjust the current implementation and avoid the existing import with side-effects but such an adjustment would NOT guarantee that other webpack/tester configurations would not trigger similar problems elsewhere.

Given that react-toastify:

I think the safest solution would be to mark it as having side-effects: it would solve our specific problem and ensure that it won't happen again :)

fkhadra commented 3 years ago

@andreifloricel thanks for the explanation. I've published a version with sideEffects set to true, you can install run yarn or npm with react-toastify@test. Can you confirm that it solves your issue? If it's the case then I'll push the fix to latest as well.

andreifloricel commented 3 years ago

@fkhadra I just tested it, everything works as expected with react-toastify@test :)

andreifloricel commented 3 years ago

@fkhadra Thank you very much! :)

fkhadra commented 3 years ago

You solved the issue :D