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

Upgrade react-popper version to 2.x #549

Closed
dleavitt opened this issue May 19, 2020 · 8 comments
Closed

Upgrade react-popper version to 2.x #549

dleavitt opened this issue May 19, 2020 · 8 comments

Comments

@dleavitt
Copy link
Contributor

This lib isn't affiliated with react-bootstrap but it does share a number of dependencies and will likely be used on a lot of the same projects. A couple of those dependencies use older versions right now (so e.g. a project using both libs will end up bundling two versions of popper.)

The major version bump for 5.0 seems like a good time to update these deps. I think only two changes are required:

  1. Bump react-overlays dep to 3.x
  2. Remove react-popper dep or bump to 2.x
@ericgio
Copy link
Owner

ericgio commented May 19, 2020

Thanks for opening this issue @dleavitt. The two items you mention are essentially the same, since react-overlays 3.x updates react-popper to 2.x. I actually did look into updating to react-popper v2 but ran into a couple issues, the main one being that it warns about elements using margins (see this RB issue).

I suppose I could upgrade react-overlays, since I'm not using anything in that lib that relies on react-popper. I'd still need to eventually upgrade react-popper itself, though, which would trigger the warning in a similar way to the RB issue cited above due to Bootstrap's CSS.

Any additional thoughts?

@dleavitt
Copy link
Contributor Author

Thanks for the background and that link! Comparing the package.json files was about as far as I got, but happy to look into this further if you're interested.

The react-bootstrap fix looks a little involved. Maybe it could be reused here.

@ericgio ericgio changed the title Align 5.0 dependency versions with react-bootstrap Upgrade react-popper to 2.x May 27, 2020
@ericgio ericgio changed the title Upgrade react-popper to 2.x Upgrade react-popper version to 2.x May 27, 2020
@ericgio
Copy link
Owner

ericgio commented Dec 9, 2020

react-overlays upgraded in v5.1.4

@edemaine
Copy link

Just bumped into this, as I'm using modern react-bootstrap (with popper 2) and would like to add this to my project, ideally with a shared popper via shared NPM dependency.

Fixing this in react-bootstrap indeed looks messy, but that's because <Overlay> is incredibly general, so the user might use margins in arbitrary ways. Presumably here you use top-level margins in very restricted ways (I just see .rbt-menu's margin-bottom) so they could be replaced with Popper's offset JavaScript setting (at the price of incompatibility, replacing CSS with JS)? I'm no Popper expert, though I could investigate further...

@ericgio
Copy link
Owner

ericgio commented Jan 20, 2021

@edemaine: thanks for providing your perspective. Part of the problem here is that, like React-Bootstrap, the typeahead relies on TWBS's CSS and that has margin styles defined.

I could just update the Popper version; as the Popper author points out in the thread I linked above, library simply triggers warnings, not errors, and apparently TWBS v5 (currently in beta) will support Popper v2. OTOH, people tend to freak out about warnings and I'll probably want to support TWBS v4 for awhile even after v5 lands (if possible).

@edemaine
Copy link

edemaine commented Jan 20, 2021

Ah, I see now, thanks. Indeed, so you pretty much have the general case of CSS out of your control. 🙁

It would be nice if it were possible to use React-Bootstrap's Overlay component when it's available (including its workaround), and directly use Popper otherwise. But I can't think of a clean way to do that, other than a fork of some sort, which would then be annoying to maintain...

Perhaps a somewhat more reasonable approach would be to have a next version of this package that uses Popper v2, which people can opt into and either suffer the warnings now or upgrade to TWBS v5. You'd still be maintaining two versions of the package until you could fully deprecate the old one, but at least it would "just" be two Popper versions (though the more I look at them, the more I see the interfaces differ substantially, so it's up to you whether this is reasonable).

Alternatively, React Bootstrap's workaround isn't looking so bad 🙂 -- it's just a query to the formatted margin.

(and FWIW, I decided to use this package in my project now anyway -- it's too good not to use!)

@evantahler
Copy link

evantahler commented Feb 6, 2021

With the new way that NPM v7 resolves peerDependecies, popper v1.x cannot be used with a react v17 project... which means that react-bootstrap-typeahead cannot be used with a react 17 project any more :(

@ericgio
Copy link
Owner

ericgio commented Oct 30, 2021

Updated in v6.0.0-alpha.4

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

4 participants