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

Remove unstable_renderIntoContainer #10143

Open
gaearon opened this issue Jul 11, 2017 · 15 comments
Open

Remove unstable_renderIntoContainer #10143

gaearon opened this issue Jul 11, 2017 · 15 comments

Comments

@gaearon
Copy link
Collaborator

gaearon commented Jul 11, 2017

My hunch is we want to remove it before 16 because unstable_createPortal accomplishes the same thing. I remember unstable_renderIntoContainer adding a bunch of complexity that would be nice to get rid of before committing to support it for another release cycle.

@gaearon gaearon added this to the 16.0 milestone Jul 11, 2017
@sebmarkbage
Copy link
Collaborator

It doesn't allow for an incremental upgrade path though. You have to upgrade all your calls at once. Even if those are in third parties. Since the portal API doesn't exist in 15. We used both at the same time so it's fair to assume others might need to. IMO we should keep it for a release and warn.

@gaearon gaearon removed this from the 16.0 milestone Jul 15, 2017
@gaearon
Copy link
Collaborator Author

gaearon commented Jul 15, 2017

We used both at the same time so it's fair to assume others might need to.

That’s fair, agree.

@aweary
Copy link
Contributor

aweary commented Sep 15, 2017

@gaearon so is the actionable item here adding a warning for unstable_renderSubtreeIntoContainer?

@gaearon
Copy link
Collaborator Author

gaearon commented Sep 17, 2017

Let's wait for popular libs to update to portal API first. Will take a few months.

@yesmeck
Copy link

yesmeck commented Sep 25, 2017

We use unstable_renderIntoContainer to implement rc-dialog, one feature is that when a dialog is unmounting, instead of calling unmountComponentAtNode, we call unstable_renderIntoContainer again to trigger a animation, after animation finished we call unmountComponentAtNode to clear the portal, see this example (check "destroy on close"). But this feature seems can't archive when using ReactDOM.createPortal, because the portal is unmount by React itself.

@gaearon
Copy link
Collaborator Author

gaearon commented Sep 25, 2017

@yesmeck Can you please file a new issue to discuss this use case?

@jquense
Copy link
Contributor

jquense commented Sep 25, 2017

That is an interesting use of the callback, very clever :) I think in the new API we should be able to handle this case the same way as other react components? E.g something like the react-transition-group Transition component.

@yesmeck
Copy link

yesmeck commented Sep 26, 2017

I'v create a new issue #10826

@raunofreiberg
Copy link
Contributor

raunofreiberg commented Apr 26, 2018

@gaearon Is there a legitimate reason for still keeping unstable_renderSubtreeIntoContainer around? Could it be deprecated or at the very least warn when using it (currently there are no warnings)?

IMHO, the inherent inconsistency of these two APIs causes a certain amount of disorientation to the whole Portals API since the unstable method is used during componentDidMount yet createPortal is used in the render method.

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 26, 2018

It's time to deprecate it. Want to send a PR?

@jquense
Copy link
Contributor

jquense commented Apr 26, 2018

It'd be nice to leave until #11387 and related are solved...At the moment there is no real good option for doing what unstable_renderSubtreeIntoContainer in cases where createPortal's event behavior is not workable (e.g. Modals)

@raunofreiberg
Copy link
Contributor

raunofreiberg commented Apr 27, 2018

Sure. I can start working on it.

@gaearon
Copy link
Collaborator Author

gaearon commented Apr 27, 2018

It doesn't seem like #11387 is moving forward so I think it makes more sense to deprecate anyway. Maybe that will create more pressure to solve #11387 :-)

@jquense
Copy link
Contributor

jquense commented Apr 27, 2018

Do you have a sense @gaearon of what is needed in #11387 to move forward? My sense was that the @sebmarkbage proposed an additional Slot api as the solution but it staled there? I'm happy to throw some use-cases or attempts to workaround the behavior together if that will help, we are getting push back in react-bootstrap on this and AFAICT, we can't effectively solve the issue without an ugly whitelist event preventDefault blocker.

@raunofreiberg
Copy link
Contributor

For the record - I assume we're currently talking about completely killing the API since both the topic name and previous discussion suggest that. What if we kick things off by actually deprecating it via warnings, yet keeping the API intact until a later release?

@necolas necolas added the React Core Team Opened by a member of the React Core Team label Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants