Fixes error when navigationBar is set back to null #4941
Conversation
Summary: Before that it was not possible to get a ref to a navigation bar (unless using Navigator's internal `_navBar` prop) Closes #3755 Reviewed By: svcscm Differential Revision: D2674315 Pulled By: nicklockwood fb-gh-sync-id: 26120f7bcbb675e8217b8bd963dcc6ed314d4ba3
|
By analyzing the blame information on this pull request, we identified @despairblue, @ericvicenti and @hedgerwang to be potential reviewers. |
| @@ -1088,7 +1088,10 @@ var Navigator = React.createClass({ | |||
| } | |||
| return React.cloneElement(this.props.navigationBar, { | |||
| ref: (navBar) => { | |||
| this.props.navigationBar.ref instanceof Function && this.props.navigationBar.ref(navBar); | |||
| var navigationBar = this.props.navigationBar; | |||
| if (navigationBar && navigationBar.ref instanceof Function) { | |||
There was a problem hiding this comment.
Wondering why navigationBar.ref instanceof Function was used instead of typeof navigationBar.ref === 'function'
|
cc @mkonicek |
|
@gre updated the pull request. |
| @@ -1088,7 +1088,10 @@ var Navigator = React.createClass({ | |||
| } | |||
| return React.cloneElement(this.props.navigationBar, { | |||
| ref: (navBar) => { | |||
| this.props.navigationBar.ref instanceof Function && this.props.navigationBar.ref(navBar); | |||
| var navigationBar = this.props.navigationBar; | |||
| if (navigationBar && typeof navigationBar.ref === "function") { | |||
|
@gre updated the pull request. |
|
This fix might actually be unsafe since it's dynamically looking up this.props.navigationBar which changes over time. Here's what I think the right fix is: _renderNavigationBar: function() {
let { navigatorBar } = this.props; // <---- keeping this element reference is the important part
if (!navigationBar) {
return null;
}
return React.cloneElement(navigationBar, {
ref: (navBar) => {
this._navBar = navBar;
if (navigationBar && typeof navigationBar.ref === 'function') {
navigationBar.ref(navBar);
}
},
navigator: this._navigationBarNavigator,
navState: this.state,
});
},Awhile back I wrote an npm package to help with this: let cloneReferencedElement = require('react-native-clone-referenced-element');
_renderNavigationBar: function() {
if (!this.props.navigationBar) {
return null;
}
return cloneReferencedElement(this.props.navigationBar, {
ref: navBar => { this._navBar = navBar; },
navigator: this._navigationBarNavigator,
navState: this.state,
});
},Either of these solutions would work I think. |
|
@gre updated the pull request. |
|
oh yeah, I was wondering this when I first read the code, keeping the same navigationBar make sense, then maybe we don't even need to internal check |
|
Calling cloneElement on null will throw an error I think, so this looks correct. |
|
@facebook-github-bot shipit |
|
Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/436748359869262/int_phab to review. |
|
Thanks @ide |
|
why is it failed? |
|
@gre There was some issue with the shipit bot. Lemme try again. |
|
@facebook-github-bot shipit |
|
Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/436748359869262/int_phab to review. |
|
is it related to the Travis build? |
|
I've no idea. Let's wait for @mkonicek to return from his vacation. He'll know why it's happening :) |
|
Thanks a ton @gre, this saved the day! 👍 |
|
should I open a new PR ? |
|
@facebook-github-bot shipit |
|
@gre updated the pull request. |
|
Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/436748359869262/int_phab to review. |
|
do you know if this land in 0.18 or later? thanks |
|
We could cherry-pick this to 0.18.0. |
Summary: This fixes a regression introduced in facebook@df70005 If you set navigationBar props (on Navigator) and then later set it back to null, it will crashes. (N.B. this should be possible as navigationBar is optional) cc satya164 Closes facebook#4941 Reviewed By: svcscm Differential Revision: D2788889 Pulled By: bestander fb-gh-sync-id: f8f1cd6cc2ce13b1b1b86fa76d3b22c26a8adb5b
Summary: This fixes a regression introduced in df70005 If you set navigationBar props (on Navigator) and then later set it back to null, it will crashes. (N.B. this should be possible as navigationBar is optional) cc satya164 Closes #4941 Reviewed By: svcscm Differential Revision: D2788889 Pulled By: bestander fb-gh-sync-id: f8f1cd6cc2ce13b1b1b86fa76d3b22c26a8adb5b
This fixes a regression introduced in df70005
If you set navigationBar props (on Navigator) and then later set it back to null, it will crashes.
(N.B. this should be possible as navigationBar is optional)
cc @satya164