Validate that getMenuProps has set the ref correctly #525
Conversation
| @@ -983,6 +990,10 @@ class Downshift extends Component { | |||
| } | |||
|
|
|||
| componentDidUpdate(prevProps, prevState) { | |||
There was a problem hiding this comment.
Should we add this to a componentDidMount as well?
There was a problem hiding this comment.
Yep that would make sense!
|
Okay, I'll do that! Hopefully I'll have time before I go on vacation on Wednesday. That would be a great idea, what do you think of using something like https://www.npmjs.com/package/warning? But I'll probably do this in another PR in that case |
|
Sounds perfect 👍 |
alexandernanberg
changed the title
|
Added tests and updated the |
src/downshift.js
Outdated
| @@ -983,6 +994,10 @@ class Downshift extends Component { | |||
| } | |||
|
|
|||
| componentDidUpdate(prevProps, prevState) { | |||
| if (this.getMenuProps.called && !this.getMenuProps.suppressRefError) { | |||
There was a problem hiding this comment.
This should probably be:
if (
this.getMenuProps.called &&
!this.getMenuProps.suppressRefError &&
preval`module.exports = process.env.BUILD_REACT_NATIVE !== 'true'`
) {so this code can be eliminated for native. Same with the above.
There was a problem hiding this comment.
I’ll fix that tomorrow! Hmm any reason why we dont do that for ’getRootProps’ as well? Im not so familiar with RN but why wouldnt we want to validate the ref?
There was a problem hiding this comment.
Yeah, I just checked and the only place we're using _rootNode is for codepaths that react native doesn't need. So we could probably do the same thing for those.
I'm thinking about making a custom macro that will shorten things for us/make these kinds of branches easier to maintain. But that'll be for later :)
There was a problem hiding this comment.
Oh okay! I've pushed the latest changes now, and I think you should be able to use --autosquash to squash them together to one commit
I like the sound of that idea!
|
Hmm this is super weird... the error says that preval is not defined. Tried adding istanbul ignore comments locally but still get the same issue also🤔 |
|
:-( I'm afraid I don't have time to look into this at the moment :-/ |
|
No worries, I’m off a couple of days but will pick this up again when I get back! |
|
Looks like everything is working now, don't really know why It did work locally but apparently not remotely.... I'll continue investigating |
…js#525) * fix: validate that getMenuProps has set the ref correctly * fixup! fix: validate that getMenuProps has set the ref correctly * fixup! fix: validate that getMenuProps has set the ref correctly
What:
Fixes #490 by using the same approach as we do in
getRootProps.Why:
Because it could lead to unexpected behavior and to avoid confusion.
How:
I'm still unsure of what the best solution for this is. I would like to make the check in the
rendermethod as we do for thegetRootProps, but I don't think that's possible because the ref hasn't setthis._menuNodeon the first render. I guess we could loop through all children but that seems unnecessary heavy/slow.So instead I make the validation/check in
componentDidUpdate, which looks to work fine.Another problem I encountered was that there is no way to check if
this._menuNodeis a composite element or not, which makes it hard to use the same validation as we do forgetRootProps.I wanted you opinion on this before I continue and add tests and update the documentation. Is there another better way that I've not thought of?
Checklist: