Tree-shaking doesn't work

[adi518]

I'm building a component library that uses TooltipTrigger (version 2.11.1). Unfortunately, it breaks tree-shaking by probably introducing some side-effects. I'm building with Rollup.

[mohsinulhaq]

can you try with the latest version and let me know if it's still the case?

[adi518]

I already tried, but I'll try again just to confirm. Hold short.

[adi518]

I upgraded to latest version and it still fails. See the following gist for the code that fails tree-shaking: https://gist.github.com/adi518/f0f1bedb9b0124a06aca2954003620fd

[mohsinulhaq]

I have a theory, maybe this line https://github.com/mohsinulhaq/react-popper-tooltip/blob/master/package.json#L18 is causing rollup to think that the library has side-effects, even though I specify that only importing CSS will have a side-effect.

Ideally, it should have worked because rollup from our build system has marked the export as pure.

What you can do is use v3.0.0, where I had temporarily removed this CSS side-effect and let me know if that helps.

[adi518]

I reinstalled at fixed version 3.0.0 and looks like the result is the same: https://gist.github.com/adi518/d7c06eb088997b691d92273328ce05e8

[mohsinulhaq]

maybe your project is using the commonjs version of the library, instead of the esm version.
can you create a sample project using react-popper-tooltip that uses rollup?

[adi518]

I'm fairly sure Rollup picks up the esm dist, but I'll create a quick repro.

[adi518]

Here's the reproduction: https://github.com/adi518/react-popper-tooltip-treeshake-reproduction

[adi518]

@mohsinulhaq ?

[mohsinulhaq]

@adi518 thanks for the detailed example, really sorry, I haven't had time to look into it. Will do as soon as I'm free.

[adi518]

Thanks, I appreciate that.

[mohsinulhaq]

https://github.com/adi518/react-popper-tooltip-treeshake-reproduction/pull/1/files
so the issue seems to be with the publish config of the ui-components library
you are not declaring external libraries to rollup, so rollup is bundling them into the main bundle, thus you lose on the tree-shake magic
you could either declare our lib as external, or use that generic function to mark all deps as external, so that they taken from the node_modules

try the build out with my changes and let me know

[adi518]

It works and I understand what the external function does, but I fail to understand why I'm losing the tree-shaking when I bundle it in. The ui-components library has many deps, not all of them are considered peer deps though. I expect my ESM bundle to have the ESM bundle of your library.

[mohsinulhaq]

I guess it's because ui-components is a library, not an application, libraries should always ship with its own code importing other code. Only applications need to bundle everything into one file. And they don't have the tree-shaking problem as they aren't going to be imported.

[adi518]

It makes sense to have a React components library set React as a peer dependency, but I don't think it makes sense to externalize any dependency. From my understanding of tree-shaking, the idea is to provide your code as an ESM build, which is then compatible with other ESM builds and can be tree-shaken. I'm going to try and fork this repository in attempt to find the side-effect(s) that impairs its tree-shake.

[mohsinulhaq]

i think there is a confusion between peer dependency and dependency

peer dependency means that the library user should provide it
and dependency means that on npm install, that package will install its own package
if you bundle a dependency in your own code, it's no longer a dependency

[adi518]

Some dependencies can be bundled in and that's fine. Also, if you think about it, any dependency that as an ESM bundle can be bundled in and it wouldn't matter because it can be shaken off in the consumer bundle.

[mohsinulhaq]

How do you decide which dependency to bundle and which one to not? Also, ESM and bundle have little overlap. I have seen that when you use ESM, you don't usually "bundle", you just let npm handle the dependencies via package.json and webpack/rollup handle tree-shaking via import/export.

[adi518]

With common sense. If I create a React component library, the consumer must have it. It's like buying an expansion product, it depends on having the base product for it to work. Same goes for this library, I expect Popper to be a peer dependency and not bundled in.

[mohsinulhaq]

Here you are describing which module should be a peer-dep and which one should be a dependency. But my question was about your statement that "some dependencies can be bundled". So we are talking only about dependencies and not peer-dependencies. My question is, which dependency do you bundle in and which dependency do you leave out in package.json itself? Do you have any example where and ESM library bundles its dependency inside its dist/index.js instead of keeping it in package.json as a dependency?

[adi518]

So I had a nice discussion with a colleague of mine about this and I finally hit the nail, realising my mistake. I totally understand why it should be externalised now and the exact difference to a peer dependency. Thanks for the great support. 👍

[adi518]

So I externalized everything, my tree-shake check passes, but my app bundle still shows Popper, despite not using it. It's getting frustrating, haha. I should emphasis it looks like an issue with Popper itself.

[mohsinulhaq]

https://github.com/adi518/react-popper-tooltip-treeshake-reproduction/pull/2/files
this seems to be happening with react-popper and/or @popperjs/core itself as well
you can raise this issue with them and get to the bottom of this
there is learning in this probably for both of us

[adi518]

I did check what Popper say about TS at the outset of this, but failed to check the imports in this library. See: https://popper.js.org/docs/v2/tree-shaking/

It seems your code imports it the wrong way, which brings the entire Popper library:

• https://github.com/mohsinulhaq/react-popper-tooltip/blob/master/src/TooltipTrigger.tsx#L6
• https://github.com/mohsinulhaq/react-popper-tooltip/blob/master/src/types.ts

[mohsinulhaq]

I only import types from "@popperjs/core" which is stripped away in the final dist build. Look at the content of the package tarball. It only has the react-popper import. But react-popper itself is importing @popperjs/core like this: https://github.com/popperjs/react-popper/blob/master/src/usePopper.js#L7

[adi518]

Then, it looks like we should start an issue with them. However this leads me to realize that every package can possibly break my tree-shake, so I think I need to find a way to seperate it away from the piece of bundle that's actually shake-able.

[mohsinulhaq]

yes, they could break, it's the same with lodash, when you do
import { isEmpty } from 'lodash';
it is not tree-shakable, hence plugins like https://github.com/lodash/babel-plugin-lodash exist to convert the imports to
import isEmpty from 'lodash/isEmpty'.
You can raise an issue on their repo for understanding this. I will thumbs up your issue 😝

[adi518]

We'll give it a shot. I know about that Lodash plugin, but Popper isn't as popular to justify such a plugin.

[adi518]

floating-ui/react-popper#380

[denisborovikov]

We just release the hook version of the library. Could you please check if this is still an issue?

[mohsinulhaq]

Anyway, as per discussion with the underlying lib owner, even if the issue still persists, it's an issue to be resolved from their side.
As such, I'm closing this issue. Feel free to take the issue further on the issue linked above.