Resolve findDOMNode deprecation by merging ScrollBlock and ScrollCaptor into ScrollManager
#4330
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 28ee545:
|
Rall3n
force-pushed
the
findDOMNode-ScrollManager
branch
from
d179d6c to
7892455
Compare
|
|
||
| setTargetRef = (instance: HTMLElement | null) => { | ||
| this.targetRef.current = instance; | ||
| this.setState({ enableLock: !!instance }); |
There was a problem hiding this comment.
If the intent is to trigger a re-render, can we store the element in state, similar to what ScrollBlock does? That way we can avoid re-rendering if the element is the same as well as avoid removing and then reattaching the listeners to the same element. I know you've said you're against storing an element in state, I'd be curious to know why.
Another option would be to make ScrollLock not a React component anymore since all it does is attach event listeners to the element and it doesn't actually render anything. It could just expose two methods, one to add listeners and one to remove them and we could call them in the appropriate lifecycle methods in ScrollManager. If I understand the situation correctly, I think this would remove the need to trigger a re-render for ScrollLock by setting state, which I would be a fan of.
There was a problem hiding this comment.
If the intent is to trigger a re-render, can we store the element in state, similar to what ScrollBlock does?
I know you've said you're against storing an element in state, I'd be curious to know why.
I learned and made it my ideology, that state is explicitly for data, while references to DOM elements and possibly React components should be kept in refs. That is why I`m against it. Period.
Another option would be to make ScrollLock not a React component anymore since all it does is attach event listeners to the element and it doesn't actually render anything.
The React mindset is all about components. And I think either would not be an improvement over the other.
There was a problem hiding this comment.
Regarding putting the DOM element in state, that's fine, I would prefer not putting DOM elements in state as well (although I think that's sometimes the best solution available, just not in this case).
While the React mindset is all about components, that doesn't mean literally every piece of logic has to live in a component. There's already precedent in react-select to take logic that makes most sense in a componentDidMount, etc. and abstracting it into a function based on utils.js.
I would also argue that the mindset of React is also against taking DOM elements received from refs and passing them down to child components through props, because it makes it difficult to make sure that those elements are properly received without forcing what should be an unnecessary re-render on the parent component. The reason why componentDidMount, etc. exist are to perform actions on DOM elements that are received via refs after the render has taken place, but that breaks down as soon as you start passing elements received from refs as props to other components.
There was a problem hiding this comment.
Also, components that always return null in the render method (like ScrollLock) don't really fit into the React mindset either.
There was a problem hiding this comment.
Back to topic: I don't think it's worth the effort to rewrite ScrollLock (for this PR). Maybe add it to the backlog?
There was a problem hiding this comment.
Sounds fine, I'll make a follow-up PR to avoid the re-render in ScrollManager and hopefully we can get them merged in back-to-back for 4.0, or if there is a problem with the follow-up PR, we can just wait to merge it in order to get this released ASAP.
Would we have to use the same pattern of having a (ref) => { } child function if it were a wrapping component? That seems like a necessary pattern in these situations, but I personally think it adds unnecessary complexity when you're doing it more than once for the same DOM element. I understand the intention to abstract the functionality into a reusable component, but I think there's a simpler way to achieve this behavior that follows good coding practices and is still in the React mindset (imo).
Yeah, using a library sounds good too. For that specific library I'm not sure I understand how it works if the child is a component and not a function because it seems like it's just getting a ref to the child component and not a ref to the DOM element (based on this line)? Although to be fair I only looked at it very quickly, so I could easily be missing something.
ebonow
added
the
menu-bug
Rall3n
force-pushed
the
findDOMNode-ScrollManager
branch
from
c6040e3 to
12a2c7a
Compare
This is an alternative PR to #4102 to resolve the
findDOMNodedeprection of #4094.As #4102 tried to solve the issue through adding the reference as a prop, this PR aims to provide a solution by merging the components
ScrollBlockandScrollCaptorinto a singleScrollManagercomponent. This component will have its own reference to the needed DOM element (instead of one through props), which resulted in thechildrenprop being a function.In its conception both
ScrollBlockandScrollCaptorwere planned to have their own references withchildrenalso being a function. But to minimize possible performance issues they resulted in being merged into theScrollManagercomponent.