Stanko / react-animate-height

Lightweight React component for animating height using CSS transitions. Slide up/down the element, and animate it to any specific height.
https://muffinman.io/react-animate-height
MIT License
756 stars 53 forks source link

Warning about `this` when using rollup and esm #136

Closed magnattic closed 1 year ago

magnattic commented 1 year ago

For our library, I am using rollup to bundle for both esm and cjs. I get the following warning during build:

yarn build

./src/index.ts → lib/cjs/index.js, lib/esm...
(!) "this" has been rewritten to "undefined"
https://rollupjs.org/troubleshooting/#error-this-is-undefined
../.yarn/__virtual__/react-animate-height-virtual-7422d79253/0/cache/react-animate-height-npm-3.1.0-d030655459-46f802fe1e.zip/node_modules/react-animate-height/dist/esm/index.js
1: var __rest = (this && this.__rest) || function (s, e) {
                 ^
2:     var t = {};
3:     for (var p in s) if (Object.prototype.hasOwnProperty.call(s, p) && e.indexOf(p) < 0)
...and 1 other occurrence
created lib/cjs/index.js, lib/esm in 19.7s

Expected behavior No warnings when bundling for esm.

Possible Solution I actually don't know what's the cause, it looks like this comes from tslib somehow? It seems that the output of the react-animate-height esm bundle includes top-level this, so maybe it's possible to change that somehow? Possibly it's also something I can change in my config.

Your Environment

Stanko commented 1 year ago

Hey, thank you for the detailed report!

I was confused at first, as I'm not using this anywhere in my code.

It seems to be coming from typescript inserting a shim for the rest operator ({ ... }):

var __rest = (this && this.__rest) || function (s, e) {

I haven't never used rollup directly (only through vite) so I'm not really familiar with it. Checking the link from the error message, maybe this can be used as a workaround:

There are occasional valid reasons for this to mean something else. If you're getting errors in your bundle, you can use options.context and options.moduleContext to change this behaviour.

But I'll look into it.

One solution is increase the target for the esm module to es2017 to exclude the rest operator shim, but I'm afraid of breaking the backwards compatibility.

If you find a solution please let me know!

magnattic commented 1 year ago

You are right, after looking deeper into this it comes from this shim. Seems pretty harmless, since the code will just fall back to using the second implementation.

I could ignore the warning, but it will probably confuse other rollup users.

An idea could be to actually use tslib during bundling of your lib to remove this specific problem (and potentially reduce the bundle size for projects using your lib): https://www.npmjs.com/package/tslib

But I'm not an expert on this and what implications this has. From what I gather this would simply import the rest function from tslib instead of polyfilling it yourself. So your bundle gets smaller for everyone that is already importing tslib in their project (pretty common).

Stanko commented 1 year ago

I'll try to see what can I do during this week 👍

Stanko commented 1 year ago

I just released v3.1.1, if you can please try it out.

https://github.com/Stanko/react-animate-height/commit/1a2428e52b1a09ebf99d2fc8efea54ef706fd9a4

I was using rest operator at a single place so I just completely removed it in the end.

magnattic commented 1 year ago

Heh, that's also a way. 😁

The new version works for me, no more warnings. thanks!