react-popper icon indicating copy to clipboard operation
react-popper copied to clipboard

Can't shake Popper

Open adi-works opened this issue 5 years ago • 12 comments
trafficstars

I consume react-popper-tooltip which is built on top of this package, and my simple CRA app can't shake it. I started an issue with the author here: https://github.com/mohsinulhaq/react-popper-tooltip/issues/97 and after looking at the code, we realized it can be an issue with react-popper, hence this issue. With Popper being shake-able, I don't see why tree-shaking shouldn't work.

Repro: https://github.com/adi518/react-popper-tooltip-treeshake-reproduction

Versions used by react-popper-tooltip:

  • Popper.js: ^2.4.4
  • react-popper: ^2.2.3

adi-works avatar Jul 25 '20 14:07 adi-works

to add to this, https://github.com/popperjs/react-popper/blob/master/src/usePopper.js#L7, here we are importing the whole package, while https://popper.js.org/docs/v2/tree-shaking/#popper-lite recommends that we should use popper-lite import to enable tree-shaking

mohsinulhaq avatar Jul 25 '20 15:07 mohsinulhaq

Have you tried to define your own createPopper option? That should allow the built in import to be tree shaked

FezVrasta avatar Jul 26 '20 09:07 FezVrasta

@FezVrasta do you mean I should recreate react-popper? Because that's all I use in the library.

mohsinulhaq avatar Jul 26 '20 10:07 mohsinulhaq

No, react-popper has a configuration option to pass your own "createPopper" function

FezVrasta avatar Jul 26 '20 12:07 FezVrasta

@FezVrasta my lib is built on top of the render-prop API, which doesn't seem to support it: https://github.com/popperjs/react-popper/blob/master/src/Popper.js#L69 Is there any way to use the "lite" createPopper with render props.

mohsinulhaq avatar Jul 26 '20 17:07 mohsinulhaq

@FezVrasta ? @mohsinulhaq Any progress on this guys? seems like an easy fix.

adi-works avatar Jul 30 '20 15:07 adi-works

Feel free to send a PR, unfortunately I don't have time to allocate to this now.

FezVrasta avatar Jul 30 '20 15:07 FezVrasta

I think we can submit a PR that changes the import of createPopper, unless you mean for a different kinda of fix.

adi-works avatar Jul 30 '20 16:07 adi-works

That would mean all the consumers would need to manually import the required modifiers, that'd be less than ideal

FezVrasta avatar Jul 30 '20 16:07 FezVrasta

Ah, so that means adding createPopper support to render props.

adi-works avatar Jul 30 '20 16:07 adi-works

Yes I think that'd work, but I'm not 100% sure tree shaking is working properly with this system either... I never tested it

FezVrasta avatar Jul 30 '20 16:07 FezVrasta

@FezVrasta I just tried using the "lite" popper import (import { createPopper } from '@popperjs/core/lib/popper-lite') for usePopper hook. But it looks like tree-shaking still doesn't work. Here's the reproduction repo: https://github.com/mohsinulhaq/react-popper-tooltip-treeshake-reproduction.

Am I doing this wrongly?

mohsinulhaq avatar Aug 02 '20 20:08 mohsinulhaq