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.

Sign up for GitHub

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

Jump to bottom

switch to ReactDOM.createPortal #201

Closed
wants to merge 3 commits into from
Closed

switch to ReactDOM.createPortal #201

wants to merge 3 commits into from

Conversation

jochenberger
Copy link
Contributor

@jochenberger jochenberger commented Sep 15, 2017 •
edited
Loading

Modal stops working in React 16 (#188). I thought this might fix it but it doesn't. It might be a good idea to do the switch anyway.

@jochenberger jochenberger mentioned this pull request Sep 15, 2017
Closed
@jochenberger
Copy link
Contributor Author

Just switching to ReactDOM.createPortal doen't help, it produces the same error.

jochenberger
jochenberger commented Sep 15, 2017
View reviewed changes
src/Portal.js
@@ -11,6 +11,9 @@ import ownerDocument from './utils/ownerDocument';
* You can think of it as a declarative `appendChild()`, or jQuery's `$.fn.appendTo()`.
* The children of `<Portal/>` component will be appended to the `container` specified.
*/

const useCreatePortal = ReactDOM.createPortal !== undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also support the deprecated ReactDOM.unstable_ createPortal? IIRC, it's only present in React 16 pre-releases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd go with "no"; just support the releases i think

@jochenberger jochenberger changed the title WIP: fix Portal in React 16 switch to ReactDOM.createPortal Sep 15, 2017
@jquense
Copy link
Member

jquense commented Sep 15, 2017 •
edited
Loading

Thanks for the help, you might want to looks at the tests in Rea t for how the new API works. It's not the same as the old one you actually call it during render()

@jochenberger
Copy link
Contributor Author

I ran the tests with React 15 and 16 and they passed. But I can have another look.

@jquense
Copy link
Member

jquense commented Sep 15, 2017

I mean look at React's tests for createPortal, not ours, for proper usage of the api :)

@jochenberger
Copy link
Contributor Author

Getting closer. The tests that require React.Children.only to throw stop working, but since that also affects PositionSpec, I think it's an unrelated issue.
There are still two other tests failing in PortalSpec though.

@jochenberger
Copy link
Contributor Author

I'm struggling with when to call _mountOverlayTarget. _mountOverlayTarget needs to find the DOM node for the container. It uses ReactDOM.findDOMNode via utils/getContainer, which means that it must not be called during render.

taion
taion reviewed Sep 22, 2017
View reviewed changes
Copy link
Member

@taion taion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change to lifecycle around creating the portal in cWM rather than cDM?

src/Portal.js
@@ -11,6 +11,9 @@ import ownerDocument from './utils/ownerDocument';
* You can think of it as a declarative `appendChild()`, or jQuery's `$.fn.appendTo()`.
* The children of `<Portal/>` component will be appended to the `container` specified.
*/

const useCreatePortal = ReactDOM.createPortal !== undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'd go with "no"; just support the releases i think

@jochenberger
Copy link
Contributor Author

cDM is too late because the overlay target needs to be initialized when we call createPortal from render.

jquense
jquense reviewed Sep 25, 2017
View reviewed changes
src/Portal.js Outdated
@@ -26,9 +29,21 @@ class Portal extends React.Component {
])
};

componentWillMount(){
if (useCreatePortal){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this also needs a canUseDOM guard here, for serverside rendering. dom-helpers exports that constant for eas of use

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

jquense
jquense reviewed Sep 25, 2017
View reviewed changes
src/Portal.js Outdated
@@ -93,7 +113,12 @@ class Portal extends React.Component {
}

render() {
return null;
if (useCreatePortal && this.props.children) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

paired with my comment above, this needs to confirm this._overlayTarget exists for the SSR case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

jquense
jquense reviewed Sep 25, 2017
View reviewed changes
test/ModalSpec.js
@@ -171,7 +171,7 @@ describe('Modal', function () {
backdrop.style.borderWidth).to.equal('3px');
});

it('Should throw with multiple children', function () {
xit('Should throw with multiple children', function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are these failing now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea. But the failure is not caused by my changes (PositionSpec fails too). I suspect, there are some changes around how errors are handled and/or reported in React 16.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #207 to track that down.

@taion
Copy link
Member

taion commented Sep 25, 2017

Given that this needs to mount its own extra portal element, I don't think you can use the cWM-based flow.

I think you need to create the element in cDM, then e.g. check this.state in render to see if we have a portal element.

@jquense
Copy link
Member

jquense commented Sep 25, 2017

So the new Portal API is awkward. We want to render the portal element on the first render if possible to avoid missing updates or bubbling issues that the older api has. But yeah, it doesn't work if the mount container is isn't just a DOM node, or in the SSR case. I'm not sure what the best option forward is...I'm leaning towards just requiring a DOM node as a container vs a component instance but that might dramatically reduce the usefulness of the component?

Side note, the behavior between the old and new API is different, createPortal, allows events to bubble through as if it was a real child

@taion
Copy link
Member

taion commented Sep 25, 2017

Wait why do we need _overlayTarget anyway here? There's no way we can safely createElement a div in cWM, is there?

@jquense
Copy link
Member

jquense commented Sep 25, 2017 •
edited
Loading

You can safely create one if you check for document in a browser it's going to be there, its only in SSR env that you can't but that's gonna give the same result as if you double rendered and created in cDM, e.g the component won't get rendered

@jochenberger jochenberger force-pushed the support-react-16 branch 3 times, most recently from 48b1132 to 2e3c4c6 Compare September 26, 2017 07:52
@jochenberger jochenberger mentioned this pull request Sep 26, 2017
Closed
@TryingToImprove
Copy link

Note that the examples does not work anymore because of @monastic.panic/component-playground/Playground which is still using React.PropTypes

@jquense
Copy link
Member

jquense commented Sep 27, 2017

I updated component-playground to fix the propType issues. What do we think about this? Have we solved the "container that is a component instance" issue?

@jquense
Copy link
Member

jquense commented Sep 27, 2017

Ok, I'm realizing there is a bunch of code here (originally) that I don't really understand any more...

@jochenberger
Copy link
Contributor Author

This is probably obsolete now that #208 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@taion taion taion left review comments

@jquense jquense jquense left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jochenberger @jquense @taion @TryingToImprove