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

Replace NodeResolver with Ref usage #4102

Closed
wants to merge 8 commits into from
Closed

Replace NodeResolver with Ref usage #4102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b482f57
Replace NodeResolver with Ref usage
Rall3n Jun 27, 2020
8464574
Merge branch 'master' into replace-findDOMNode
bladey Jun 30, 2020
f1d918b
Add `prevProps` typing
Jun 30, 2020
516da65
Merge branch 'master' into replace-findDOMNode
Rall3n Nov 23, 2020
bba85ad
Resolve flow typing issue
Rall3n Nov 30, 2020
fa1c810
Merge branch 'master' into replace-findDOMNode
bladey Dec 2, 2020
18a6bb2
Merge branch 'master' into replace-findDOMNode
Rall3n Dec 10, 2020
0aba924
Add `null` check in `ScrollCaptor.stopListening`
Rall3n Dec 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/react-select/src/Select.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1745,8 +1745,9 @@ export default class Select extends Component<Props, State> {
isEnabled={captureMenuScroll}
onTopArrive={onMenuScrollToTop}
onBottomArrive={onMenuScrollToBottom}
targetRef={this.menuListRef}
>
<ScrollBlock isEnabled={menuShouldBlockScroll}>
<ScrollBlock isEnabled={menuShouldBlockScroll} targetRef={this.menuListRef}>
<MenuList
{...commonProps}
innerRef={this.getMenuListRef}
Expand Down
21 changes: 0 additions & 21 deletions packages/react-select/src/internal/NodeResolver.js
View file
Open in desktop

This file was deleted.

24 changes: 4 additions & 20 deletions packages/react-select/src/internal/ScrollBlock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,30 +2,20 @@
/** @jsx jsx */
import { PureComponent, type Element } from 'react';
import { jsx } from '@emotion/core';
import NodeResolver from './NodeResolver';
import ScrollLock from './ScrollLock/index';

type Props = {
children: Element<*>,
isEnabled: boolean,
};
type State = {
touchScrollTarget: HTMLElement | null,
targetRef: HTMLElement,
};

// NOTE:
// We shouldn't need this after updating to React v16.3.0, which introduces:
// - createRef() https://reactjs.org/docs/react-api.html#reactcreateref
// - forwardRef() https://reactjs.org/docs/react-api.html#reactforwardref

export default class ScrollBlock extends PureComponent<Props, State> {
state = { touchScrollTarget: null };

// must be in state to trigger a re-render, only runs once per instance
getScrollTarget = (ref: HTMLElement) => {
if (ref === this.state.touchScrollTarget) return;
this.setState({ touchScrollTarget: ref });
};
export default class ScrollBlock extends PureComponent<Props> {

// this will close the menu when a user clicks outside
blurSelectInput = () => {
Expand All @@ -35,8 +25,7 @@ export default class ScrollBlock extends PureComponent<Props, State> {
};

render() {
const { children, isEnabled } = this.props;
const { touchScrollTarget } = this.state;
const { children, isEnabled, targetRef: touchScrollTarget } = this.props;

// bail early if not enabled
if (!isEnabled) return children;
Expand All @@ -46,11 +35,6 @@ export default class ScrollBlock extends PureComponent<Props, State> {
* ------------------------------
* blocks scrolling on non-body elements behind the menu

* NodeResolver
* ------------------------------
* we need a reference to the scrollable element to "unlock" scroll on
* mobile devices

* ScrollLock
* ------------------------------
* actually does the scroll locking
Expand All @@ -61,7 +45,7 @@ export default class ScrollBlock extends PureComponent<Props, State> {
onClick={this.blurSelectInput}
css={{ position: 'fixed', left: 0, bottom: 0, right: 0, top: 0 }}
/>
<NodeResolver innerRef={this.getScrollTarget}>{children}</NodeResolver>
{children}
{touchScrollTarget ? (
<ScrollLock touchScrollTarget={touchScrollTarget} />
) : null}
Expand Down
32 changes: 16 additions & 16 deletions packages/react-select/src/internal/ScrollCaptor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,27 +2,31 @@

import React, { Component, type Element } from 'react';

import NodeResolver from './NodeResolver';

export type CaptorProps = {
children: Element<*>,
onBottomArrive?: (event: SyntheticEvent<HTMLElement>) => void,
onBottomLeave?: (event: SyntheticEvent<HTMLElement>) => void,
onTopArrive?: (event: SyntheticEvent<HTMLElement>) => void,
onTopLeave?: (event: SyntheticEvent<HTMLElement>) => void,
targetRef: HTMLElement,
};

class ScrollCaptor extends Component<CaptorProps> {
isBottom: boolean = false;
isTop: boolean = false;
scrollTarget: HTMLElement;
touchStart: number;

componentDidMount() {
this.startListening(this.scrollTarget);
this.startListening(this.props.targetRef);
}
componentDidUpdate(prevProps: CaptorProps) {
if (prevProps.targetRef != this.props.targetRef) {
if (prevProps.targetRef) this.stopListening(prevProps.targetRef);
this.startListening(this.props.targetRef);
}
}
componentWillUnmount() {
this.stopListening(this.scrollTarget);
this.stopListening(this.props.targetRef);
Copy link
Collaborator

@Methuselah96 Methuselah96 Dec 9, 2020 •
edited
Loading

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.

Copy link
Collaborator Author

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 in componentDidMount().

This lifecycle is made for such things, so I am pretty confident that the reference will never be null at this point.

Copy link
Collaborator

@Methuselah96 Methuselah96 Dec 9, 2020 •
edited
Loading

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

@Methuselah96 Methuselah96 Dec 11, 2020 •
edited
Loading

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.

Copy link
Collaborator Author

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.

Copy link
Collaborator

@Methuselah96 Methuselah96 Dec 11, 2020 •
edited
Loading

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:

  1. 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 the MenuPlacer or the functionality of MenuPlacer changes that it would be very easy to miss the fact that that would cause the ScrollBlock and ScrollCaptor to no longer work (since it's not obvious that these components depend on the functionality of Menu). 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.
  2. 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.
  3. This functionality is not tested, so it would be easy for it to break without anyone noticing.
  4. My code snippet was completely based on logic that was already in ScrollBlock (putting the ref in state in order to force a re-render), I just put it in a higher component so that it would apply to both ScrollBlock and ScrollCaptor. (I tried having each component put the ref in state, but it would be hard to make sure that the innerRef passed into MenuList 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.

Copy link
Collaborator

@Methuselah96 Methuselah96 Dec 11, 2020 •
edited
Loading

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?

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

}
startListening(el: HTMLElement) {
// bail early if no element is available to attach to
Expand All @@ -40,6 +44,9 @@ class ScrollCaptor extends Component<CaptorProps> {
}
}
stopListening(el: HTMLElement) {
// bail early if no element is available to detach from
if (!el) return;

// all the if statements are to appease Flow 😢
if (typeof el.removeEventListener === 'function') {
el.removeEventListener('wheel', this.onWheel, false);
Expand All @@ -62,9 +69,10 @@ class ScrollCaptor extends Component<CaptorProps> {
onBottomLeave,
onTopArrive,
onTopLeave,
targetRef,
} = this.props;
const { scrollTop, scrollHeight, clientHeight } = this.scrollTarget;
const target = this.scrollTarget;
const { scrollTop, scrollHeight, clientHeight } = targetRef;
const target = targetRef;
const isDeltaPositive = delta > 0;
const availableScroll = scrollHeight - clientHeight - scrollTop;
let shouldCancelScroll = false;
Expand Down Expand Up @@ -116,16 +124,8 @@ class ScrollCaptor extends Component<CaptorProps> {
this.handleEventDelta(event, deltaY);
};

getScrollTarget = (ref: HTMLElement) => {
this.scrollTarget = ref;
};

render() {
return (
<NodeResolver innerRef={this.getScrollTarget}>
{this.props.children}
</NodeResolver>
);
return (this.props.children);
}
}

Expand Down
1 change: 0 additions & 1 deletion packages/react-select/src/internal/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,5 @@

export { default as A11yText } from './A11yText';
export { default as DummyInput } from './DummyInput';
export { default as NodeResolver } from './NodeResolver';
export { default as ScrollBlock } from './ScrollBlock';
export { default as ScrollCaptor } from './ScrollCaptor';