React-modal: React 16 compatibility

[amannn]

Summary:

I looked around in the issue tracker but couldn't find an existing issue. As React 16 is right around the corner, maybe it's time to prepare a PR that adds support for it?

It looks like at least the call to unstable_renderSubtreeIntoContainer should be updated as there's now first class support for portals.

Steps to reproduce:

1. Render a modal.
2. Unmount it.

Expected behavior:

Shouldn't throw any errors.

Link to example of issue:

https://codesandbox.io/s/xpl8r853o4

[diasbruno]

Hey @amannn. I think I'll join this issue and #484, since both refers unstable_renderSubtreeIntoContainer as a main problem.

Right now, I'm mostly moderating the repo (currently, I don't have much time to get some real work here) and trying to answer as many questions as I can.

If you have time, you can start a PR and I can provide some assistance.

[amannn]

Hey @diasbruno. Oh, I was searching with an issue with "16" in the name, therefore I didn't find the other issue – sorry.

Sure, I can make an initial PR.

Feel free to close this issue, as apparently it's redundant.

[diasbruno]

I think they are related. I guess we can merge both issues and go for a "major" improvement in react-modal. I think it's time to open a new roadmap.

[amannn]

I just had a look at the code base and it seems like there are quite a few linting errors. It's not errors from the linting task, but the eslint config finds some issues in the test code (e.g. expecting ReactDOM.render to return a reference to the top level component instance). I think those should probably be cleaned up first. Maybe somebody more familiar with the code base wants to do that?

From there it should be pretty easy to add React 16 support I guess.

[diasbruno]

We don't need to worry with linting the tests scripts unless this can be something that can compromise the test suite.

[amannn]

Well there are for example linting errors warning about deprecated APIs
like ReactDOM.render returning a reference. These will throw with an
upgrade to React 16. I thought maybe its a good idea to split this in 2
PRs: resolving those errors and the upgrade itself so it might be easier to
review.

[diasbruno]

@amannn Fair enough. I'll clear those lint problems.

[pleunv]

I had a quick look because I want to upgrade internally to React 16 and noticed closing of the modal is broken in combination with react-router (can be reproduced in the examples).

The changes necessary to make everything work with portals in r16 are pretty trivial, as you can see in the commit here: https://github.com/CogniStreamer/react-modal/commit/4434c4aac57d91a24d8f1b89e85a41fe2bd2023d

Some things:

• For some reason node = document.createElement('div'); needed to be moved out of componentDidMount, otherwise it doesn't work in combination with react-router. Does work in the other use cases. Haven't figured out why. Made it a class property for now.
• Right now it's only compatible with v16. Making it backwards compatible (so it doesn't require a major bump) should be possible but will need some wrapping trickery with ReactDOM.unstable_renderSubtreeIntoContainer. Don't have time tonight to look into this.
• 2 tests are broken:
  
  
  
  • keeps the modal in the DOM until closeTimeoutMS elapses
  
  
  • shouldn't throw if forcibly unmounted during mounting
  
  

If you want I can create a pull request so this can be used as a base.

[diasbruno]

@pleunv Thank you so much for this info! I've created a branch called next, where we can direct all the necessary stuff for react 16. I've spend sometime doing some refactoring on the tests and examples, and I think we are ready to work. Any other problems we find, we can work on this branch.

It should be a simple "sprint" and people can start using/testing from this branch (react-modal@next).

[tomasztomys]

@pleunv which version of react-router are you using? I have 3.2.0 and no problem now.

[pleunv]

You mean 4.2.0?

If you upgrade this repo to [email protected] and [email protected], then run the examples and open the react-router example you'll notice that you can't close the modal anymore after opening it. At least that's what I'm getting both here and in my project, would be weird if you could not reproduce it.

If you use the modal statefully everything is going to work, if you open or close it based on the current active <Route /> it's not. The <Modal /> component gets unmounted but the separate DOM tree doesn't get cleaned up. Reason for this is that this.portal equals null and the removePortal() is never called.

[nickhudkins]

Hey @diasbruno, would love to pitch in here to get React 16 support across the line. looks like react-modal@next has done a bunch of the cleanup work in preparation, but didn't want to get in the way of in progress work.

[diasbruno]

@pleunv @nickhudkins @amannn @tomasztomys I've started the PR. Appreciate if you can reviews it.

[catamphetamine]

I modified @diasbruno PR to make it compatible with any version of React:
https://github.com/reactjs/react-modal/pull/513
Didn't test it, so if anyone thinks he/she wants to do that then he/she can do that.
It's mostly just an illustration of the non-breaking approach.

[tomasztomys]

@pleunv no, I mean 3.2.0 :)

[jasonwilliams]

Just saw https://github.com/reactjs/react-modal/pull/513 and it looks like the react 16 fix is being rolled into version 3.0.0
Do you know what the ETA on 3.0.0 is? As this is the only module holding us up from migrating to React 16 (and i'm guessing many others to 😃 )

[phiresky]

I'm too impatient so I made https://github.com/phiresky/react-modal which is just v2.3.2 with #513 applied and built. If you're lazy and use react 16 too you can change your dependency to "react-modal": "phiresky/react-modal" until this is fixed here.

[diasbruno]

[email protected] is available.

Version 2.3.3 is the stable version and can be installed with react-modal@latest or just react-modal.

To use the compatible version with React 16, you can install by running npm install react-modal@next.

[diasbruno]

Released v3.0.0-rc1 to fix a conditional on canUseDOM to work on server-side.

[plemarquand]

@diasbruno Does 3.0.0 currently support React 16 or just the next branch?

[diasbruno]

The branch next is our "master" for react 16 support and all rc tags will be created from it. Once it become stable, it will be merged into master.

[diasbruno]

If installed with react-modal@next, npm/yarn will download the latest tag of v3.0.0-rcN. See npm help dist-tags.

[diasbruno]

Released official v3.0.

[diasbruno]

I guess we can finally close this issue.

Thank you all for the ideas, contributions and everything else.
Sorry that it took sometime to get react-modal available on react 16.