-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Replace NodeResolver with Ref usage #4102
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 0aba924:
|
Thank you so much for this PR @Rall3n. |
Hey @Rall3n, It looks like the tests are failing (9 errors):
If I remove It eliminates all errors but one:
Which seems to be a missing type for the new I may be completely off the mark here for all of the above, but I wanted to give it a go. Let me know! |
Hey @bladey, I was not sure how to type a legacy The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be checked by @JedWatson
Thanks @Rall3n! Looks good, I'll leave it to @JedWatson to check the two comments above for the tests to start passing or if we need further work. |
@bladey @JedWatson Any information regarding this pr / the failed checks for the reference typing? |
} | ||
componentWillUnmount() { | ||
this.stopListening(this.scrollTarget); | ||
this.stopListening(this.props.targetRef); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we be sure that this.props.targetRef
is not null
here? To be honest, I've never quite understood when a ref can or can't be null
, but based on the documentation I'm wondering if it could be null
in this scenario:
React will call the ref callback with the DOM element when the component mounts, and call it with null when it unmounts. Refs are guaranteed to be up-to-date before componentDidMount or componentDidUpdate fires.
I'd be happy to get the Flow types working with | null
if that's helpful. I have it working on a local branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, componentWillUnmount
is neither componentDidMount
or componentDidUpdate
.
componentWillUnmount()
is invoked immediately before a component is unmounted and destroyed. Perform any necessary cleanup in this method, such as invalidating timers, canceling network requests, or cleaning up any subscriptions that were created incomponentDidMount()
.
This lifecycle is made for such things, so I am pretty confident that the reference will never be null at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree that componentWillUnmount
is usually a good place to remove event listeners from refs, etc. However, this is not a normal ref situation since the grandparent has a ref to the grandchild and is passing it into the child as a prop. Therefore, I'm more wary than usual.
I made a CodeSandbox to see how this situation would behave, and it is possible to have a null
value in componentWillUnmount
if the top-level parent doesn't re-render (if you press "Toggle mount status of ReactSelect" immediately after loading the page). However, if the component is re-rendered (by pressing "Set state on ReactSelect") the ref will be set in the props in componentWillUnmount
.
It looks like the Select
component in react-select
is always re-rendered by something pretty quickly, but I don't think it's worth relying on that being the case. That's why I'd rather be safe and just check for null
where possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example seems very specific, but the reasoning is understandable.
I will push a commit with a null
-check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'm sorry if my concerns are coming off as pedantic, just trying to make sure we catch the edge cases.
After thinking about this some more, I realized that this means that if ScrollBlock
and ScrollCaptor
fail to re-render after the menuListRef
in Select
has been set, then the scroll blocking and capturing won't work correctly. I've been trying to figure out whether we know for sure if those re-renders will happen or not (they seem to re-render in every case I've tried), but I'm having trouble proving it one way or the other. Do you have any insight?
If we can't know for sure that those components will re-render, I was thinking that a pretty safe way to make sure they re-render would be to introduce a ScrollManager
component where the ref
is in state
, so it will re-render on state change. Something like this:
type Props = {
captureMenuScroll: boolean,
onMenuScrollToBottom?: (event: SyntheticEvent<HTMLElement>) => void,
onMenuScrollToTop?: (event: SyntheticEvent<HTMLElement>) => void,
menuShouldBlockScroll: boolean,
children: Element<*>,
};
export default function ScrollManger({
captureMenuScroll,
onMenuScrollToBottom,
onMenuScrollToTop,
menuShouldBlockScroll,
children,
}: Props) {
const childElement = Children.only(children);
const { innerRef } = childElement.props;
// must be in state to trigger a re-render, only runs once per instance
const [scrollTarget, setScrollTarget] = useState();
const refCallback = useCallback(
element => {
innerRef(element);
if (element === scrollTarget) return;
setScrollTarget(element);
},
[innerRef, scrollTarget]
);
return (
<ScrollCaptor
isEnabled={captureMenuScroll}
onTopArrive={onMenuScrollToTop}
onBottomArrive={onMenuScrollToBottom}
scrollTarget={scrollTarget}
>
<ScrollBlock
isEnabled={menuShouldBlockScroll}
scrollTarget={scrollTarget}
>
{React.cloneElement(childElement, { innerRef: refCallback })}
</ScrollBlock>
</ScrollCaptor>
);
}
Still looking into it, let me know if you have any ideas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your code snippet would be too over engineered, just simple overkill. I believe a better way would be to let the components have their own reference to the DOM Element, and not through props.
But I also think this should be part of another discussion/PR. I already have something in mind and will get to it.
The sole focus of this PR is to remove deprecated methods without changing current functionality, and I think this has been accomplished. Everything else is just preventing this from being merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely agree that my proposed solution is not ideal and that letting the components have their own reference to the DOM element would be much better (I would be interested to see how these components could get a ref to the DOM node without getting it through innerRef
). And I also completely agree on what the focus of this PR should be (although I still think having a technical discussion on how to accomplish the goal is still appropriate).
My reasons for preferring a solution that explicitly causes a re-render are:
- The current solution in this PR relies on implicit behavior of the
MenuPlacer
component working a certain way. This means if these components are either no longer a child of theMenuPlacer
or the functionality ofMenuPlacer
changes that it would be very easy to miss the fact that that would cause theScrollBlock
andScrollCaptor
to no longer work (since it's not obvious that these components depend on the functionality ofMenu
). Making the re-render explicit would make it more likely that we won't break this functionality by mistake when working with parts of the code that aren't related to the scroll components. - It's hard to predict how long a certain piece of code is going to live for or in what context someone's going to try to use your code (unless there's an active PR that is about to get merged). Therefore, if there's an easy solution that is a better long-term solution, I usually prefer the better long-term solution.
- This functionality is not tested, so it would be easy for it to break without anyone noticing.
- My code snippet was completely based on logic that was already in
ScrollBlock
(putting theref
instate
in order to force a re-render), I just put it in a higher component so that it would apply to bothScrollBlock
andScrollCaptor
. (I tried having each component put theref
in state, but it would be hard to make sure that theinnerRef
passed intoMenuList
isn't called multiple times in that situation.)
As you noted, if both of us think our approach is the better solution, then this discussion is holding up the merging of this PR. Thanks for bearing with me while I figure out how some of these internal components work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I gave up too easily on making ScrollBlock
and ScrollCaptor
responsible for getting the DOM node themselves. I came up with a solution that doesn't rely on a ScrollManager
, is a minimal code change, and handles the ref
internally inside of ScrollBlock
and ScrollCaptor
(which means that we're no longer relying on the Menu
state change to trigger a re-render). Can you take a look and see what you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like there ought to be some HOC or React hooks way to do this generically, but I can't seem to figure it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I´m not a fan of using cloneElement
to force extra props onto a component. And I´m guaranteed not a fan of storing refs in the state.
I agree that the components should handle the references by themselves, but not like this. Instead I would suggest a solution using React.createRef
and using a function as a child (much like the MenuPlacer
component) to forward the ref to the DOM element.
<ScrollCaptor>{scrollTargetRef => (
<ScrollBlock>{blockTargetRef => (
<div ref={instance => {
this.getMenuListRef(instance)
scrollTargetRef.current = instance;
blockTargetRef.current = instance;
}}
>
{ ... }
</div>)}
</ScrollBlock>)}
</ScrollCaptor>
This makes the components independent of the menuListRef
reference and React guarantees that the references are set before the lifecycle methods are called (componentDidMount
, componentDidUpdate
).
Tested it without the MenuPlacer
and Menu
components to prevent possible re-renders. Both elements worked as expected.
But as I already said and will say again: This is a matter for another PR. Get this merged to resolve StrictMode
errors as soon as possible.
Replaced by #4330. |
With React 16.x and
StrictMode
, the usage offindDOMNode
has been deprecated.This PR replaces the
NodeResolver
component, which used the deprecated function with an already existing Ref to the nearest DOM node of theMenuList
component.Related to #4094.