Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't shake Popper #380

Open
adi518 opened this issue Jul 25, 2020 · 12 comments
Open

Can't shake Popper #380

adi518 opened this issue Jul 25, 2020 · 12 comments

Comments

@adi518
Copy link

adi518 commented Jul 25, 2020

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: mohsinulhaq/react-popper-tooltip#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
@mohsinulhaq
Copy link
Contributor

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

@FezVrasta
Copy link
Member

FezVrasta commented Jul 26, 2020

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

@mohsinulhaq
Copy link
Contributor

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

@FezVrasta
Copy link
Member

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

@mohsinulhaq
Copy link
Contributor

@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.

@adi518
Copy link
Author

adi518 commented Jul 30, 2020

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

@FezVrasta
Copy link
Member

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

@adi518
Copy link
Author

adi518 commented Jul 30, 2020

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

@FezVrasta
Copy link
Member

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

@adi518
Copy link
Author

adi518 commented Jul 30, 2020

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

@FezVrasta
Copy link
Member

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

@mohsinulhaq
Copy link
Contributor

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants