-
Notifications
You must be signed in to change notification settings - Fork 228
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
Improve ref support on connectToStores (and remove refs from other components) #696
Improve ref support on connectToStores (and remove refs from other components) #696
Conversation
Are refs not needed anymore? I forget why they were there in the first place. |
That was the question that I was making myself all the time since the only place that I saw it was in the tests. Buuut, I took a second look and I think we needed the refs so we could have access to the wrapped component instance or DOM in someway. Maybe one could have a connected input and need to focus it through a ref and maybe this ref mechanism that was implemented was allowing it. If that is the case, probably I'll make some experiments here and see what I find. But maybe something that I said already rang some bells on you... Any other idea? |
Yup, I think I've figured out. The current refs allow the parent component to have access to the wrapped component through the I did the experiments with all variations of inputs and refs. You can check it live here and the source here. As you can see, we currently don't support // master branch (uses deprecated string ref)
this.ref.current.wrappedElement.focus()
// dev branch
this.ref.current.wrappedElementRef.current.focus() My proposal is to follow What they do is a conditional version of what is presented in the React docs regarding refs and hocs. You can check the relevant code from them here, but basically what they do is, if the user sets Said that, my proposal is to adopt the same approach as them. The relevant part of function connectToStores(Component, stores, getStateFromStores, options) {
class StoreConnector extends React.Component {
render() {
const { fluxibleRef, ...props } = this.props;
return <Component ref={fluxibleRef} {...props} {...this.state} />;
}
}
if (options.forwardRef) {
return React.forwardRef((props, ref) => (<StoreConnector fluxibleRef={ref} {...props} /> ));
}
return StoreConnector;
} The user code could be: // CustomInput.js
export default connectToStores(CustomInput, stores, getStateFromStores, { forwardRef: true })
// App.js
const App = () => {
const ref = React.useRef(null);
return (
<div>
<CustomInput ref={ref} />
<button onClick={() => ref.current.focus()}></button>
</div>
);
}; As you can see, we wouldn't have any ref at all in By the way, I would only keep/fix the ref support on @redonkulus what do you think? Does it make sense/was it clear? |
Thank you for the detailed analysis. Your recommendation makes sense to me and follows what other projects have done. I'm fine with this unless @mridgway or @lingyan have any reservations. I do not remember all the use cases for using ref's via connectToStore but making it an option and pass through prop makes sense to me. As for |
Oh, I learned a lot during this analysis, so, no need to say thanks :) I've updated the branch with my proposal so you can take a better look on how things could be. |
@redonkulus we could also add it back later as a "bug fix" if we see the need. This would be faster than creating another major release. |
@pablopalacios it looks like React has some documentation about optionally forwarding the ref, looks like we should always do it. Which means the extra option would not be necessary:
|
Sure! The only problem is that we must not insert the ref when it's a functional component but still insert refs in forwardref components. I think |
@redonkulus I tried it and meh... It didn't work. The problem is that This means that there is no clean way to check if the component that must be wrapped by
My opinion is that React documentation is a little bit incomplete regarding HOCs and forward refs. It assumes that all wrapped components will be "ref-able" components. If we don't want to play dirty, the only way that I see to circumvent this issue is by using some kind of flag that the user controls as proposed before. One thing that we could do differently is, instead of using function connectToStores(Component, stores, getStateFromStores, options) {
class StoreConnector extends ReactComponent {
// ...
render() {
const ref = options.forwardRef ? this.props.fluxibleRef : null;
return <Component ref={ref} />
}
}
// ...
return React.forwardRef((props, ref) => <StoreConnector fluxibleRef={ref} />);
} In this way we would always have a forwardRef component (dismissing the problem of conditionally applying it as discouraged by react docs) but we would leave to the user to decide if a ref should be forward or not (eliminating the need to introspect the component). One benefit that I see here is that the API will likely survive react updates: if they decide to create any other ref-able component, we don't have to update any thing on our side. |
I like this approach and can future proof us against React changes. |
@redonkulus updated :) |
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.
This is great, I like the changes, seems clear to me. Thanks for doing the hard work!
@pablopalacios new beta versions published to npm |
addons-react/connectToStores
wrappedElementRef
refReact.forwardRef
with{ forwardRef: true }
router/handleHistory
wrappedElement
refrouter/handleRoute
wrappedElement
refaddons-react/batchedUpdatePlugin
addons-react/provideContext
wrappedElementRef
refI confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.