4.1.0 introduces a breaking change and should be revoked (and re-released as 5.0.0) about react-tiny-popover HOT 14 CLOSED

Version 5 is now out. Thank you guys for your patience. I'l go ahead and close this for now, but feel free to reopen if something is behaving oddly or remains unclear.

Thanks!

from react-tiny-popover.

Thank you for bringing this to my attention. I'll have this version revoked and release a major version update, likely changing Popover's child to a render function injected with a ref. That pattern, although slightly more complex, seems to be the best way to replace findDOMNode's functionality without adding anything like an extra div to the DOM. I do wonder though if a better way exists.

In any case, stay tuned for that update and associated docs. Apologies for the inconvenience in the interim.

from react-tiny-popover.

Yep just broke my app after upgrading.

I can't use wrap children with elements.

Looking forward to the fix.

from react-tiny-popover.

@volkandkaya this can be addressed in userland just by wrapping your component child in a forwardRef and passing the ref through to the host node manually.

from react-tiny-popover.

Correct. The fix will require passing the ref manually regardless, so any functional component used as a Popover child will now have to consume an injected ref via ref forwarding.

This is a consequence of React phasing out the imperative nature of findDOMNode in favor of the declarative nature of refs (which I honestly approve of philosophically). I'm struggling to see if there's a way to accommodate functional components without inserting an extra div or injecting a ref to forward. I don't think there's any other proper way in Strict mode. Am I correct in this thinking?

from react-tiny-popover.

@alexkatz your problem essentially is there's no way to force a child to have a prop if the child is not properly setup to forward those props to a host node – even if you used IDs or classNames instead of refs (which I wouldn't advise), you're going to rely on the user forwarding that too.

The only way I can think of avoiding having the user forward props (/ref) is to have a container that you control. But even that's probably a breaking change.

from react-tiny-popover.

If it isn't possible that is fine just have some nice docs to help people swap.

And maybe up it to 5.0.0 as @nathggns said.

I used this library because it just worked.

I tried a bunch of other ones that didn't.

from react-tiny-popover.

@volkandkaya Yep, stay tuned for the update and associated docs. Working on it now.

I'm bummed that findDOMNode seemed to have this unique functionality that isn't possible in Strict mode. I guess that's just the path React is taking as a framework, which ultimately I approve of. findDOMNode was a rather hacky approach to begin with that kind of violated the principles of React itself. It certainly simplified things though in this case.

from react-tiny-popover.

I'm bummed that findDOMNode seemed to have this unique functionality that isn't possible in Strict mode. I guess that's just the path React is taking as a framework, which ultimately I approve of. findDOMNode was a rather hacky approach to begin with that kind of violated the principles of React itself. It certainly simplified things though in this case.

@alexkatz I completely get where you're coming from. I guess the React team's argument would be that this change is making what was previously an unsafe implicit requirement, safe and explicit (that there is a single host node rendered to represent the containing box to attach the popover to). If you go down the supply the ref as a render prop method, then you're also giving the user the ability to specify exactly which host node to attach the popover to, rather than the implicit requirement of it being a single direct descendent.

from react-tiny-popover.

@nathggns Yup, you're right. Ultimately it's better that way, even if it makes this popover API slightly more cumbersome.

from react-tiny-popover.

Hi @alexkatz.

I just did a minor version bump, and this broke our app. Could you please redact the 4.1.0 release since it is clearly breaking it's API? I mean it's an easy enough fix, and we'll just jump to version 5 now, -- but it is not a good practice to change API in a minor version.

If you cannot disable the 4.1.0 release on npm, maybe create a branch of the old 4.0.0 as 4.2.0 so anyone doing an minor update won't break their code like I did? :)

Thanks for your work on this!

from react-tiny-popover.

Shit, it is deprecated, but yarn upgrade still upgraded to it. So yarn is the bad boy here.

from react-tiny-popover.

I think re-releasing 4.0 as 4.2 is a good idea

from react-tiny-popover.

Indeed, or even 4.1.1 would work.

Yarn did print the warning, but in a huge project those are hard to see, since it scrolls out of the screen right away with other output. :)

from react-tiny-popover.