[core] Remove direct references to window/document objects #10328
Conversation
|
oliviertassinari
force-pushed
the
document-refs
branch
from
6453272
to
3676404
Compare
src/Modal/Modal.js
Outdated
| function getOwnerDocument(element) { | ||
| return ownerDocument(ReactDOM.findDOMNode(element)); | ||
| function getOwnerDocument(container, modalInstance) { | ||
| const node = container || ReactDOM.findDOMNode(modalInstance); |
There was a problem hiding this comment.
Why do we need || ReactDOM.findDOMNode(modalInstance)? I would have expected this.mountNode to be enough.
There was a problem hiding this comment.
I think you are correct. I was originally using the this.props.container which ended up being troublesome. I'll update this.
There was a problem hiding this comment.
Damn you're fast! Was just in the middle of fixing :P
oliviertassinari
changed the title
oliviertassinari
force-pushed
the
document-refs
branch
from
3676404
to
3c72147
Compare
src/utils/withWidth.js
Outdated
| target="window" | ||
| onResize={this.handleResize} | ||
| ref={node => { | ||
| this.mountNode = node; |
There was a problem hiding this comment.
I'm second guessing this. Maybe i'm just tired. Let me double check that this works as expected.
There was a problem hiding this comment.
this.mountNode will be a reference to the EventListener instance. I am then doing ownerDocument(this.mountNode) which is wrong since EventListener will never have an ownerDocument property on it.
src/utils/withWidth.js
Outdated
| @@ -1,4 +1,5 @@ | |||
| import React from 'react'; | |||
| import { findDOMNode } from 'react-dom'; | |||
There was a problem hiding this comment.
For consistency: import ReactDOM from 'react-dom';
src/utils/withWidth.js
Outdated
| handleResize = debounce(() => { | ||
| this.updateWidth(window.innerWidth); | ||
| const win = ownerWindow(ReactDOM.findDOMNode(this.mountNode)); |
There was a problem hiding this comment.
This appears to work better. I'm still not 100% confident in its function.
findDOMNode(this.mountNode) always returns null from componentDidMount when a width or initialWidth prop isn't provided (such as in the docs), since we render null when width isn't defined yet in props or state.
There was a problem hiding this comment.
You are right. The logic is broken. I'm surprised the CI is green.
There was a problem hiding this comment.
Maybe you can leave the withWidth file unchanged so we can merge the other changes right away?
There was a problem hiding this comment.
Can do. When findDOMNode returns null the ownerWindow function just defaults to returning the global window object.
oliviertassinari
changed the title
|
Thanks. The |
Continuing my crusade to remove all references to global window/document objects! 🎉
Only remaining pieces I've found are the
EventListenercomponents, which pass atarget="window"prop. I tried looking to see if i could improve this from within the react-event-listener source code, but didn't come up with a solution there.