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.

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

If application root is reinitialized, update ActionSheet and Toast refs #1700

Merged
merged 2 commits into from Jun 1, 2018
Merged

Conversation

fredmon3
Copy link
Contributor

@fredmon3 fredmon3 commented Mar 16, 2018

This PR is related to issues #1397, #1315, #937.

There is potential that React Native may unmount and remount the root node of the application due to Android lifecycle events without clearing the JS execution environment. (This is possible even if native-base's Root is the top level component.) This results in actionSheetInstance being invalid as it pointed to the previously mounted application root's ActionSheet component that was unmounted.

This pull request updates the static actionSheetInstance and toastInstance refs regardless of what state they are in such that stale refs will be overwritten appropriately.

According to the React Ref documentation:

React will call the ref callback with the DOM element when the component mounts, and call it with null when it unmounts.

Thus, there is a null check on c in the ref callbacks.

@nachofregeiro
Copy link

Will this pull request be approved soon to fix both ActionSheet and Toast issues?

@fredmon3
Copy link
Contributor Author

fredmon3 commented Apr 5, 2018

If there's any additional information that I can provide to assist with this pull request, please let me know. Thank you!

@SunburtReynolds
Copy link

Is there an ETA on this PR?

@adamawang
Copy link

+1

3 similar comments
@akemys
Copy link

akemys commented Apr 25, 2018

+1

@NappyPirate
Copy link

+1

@galo2002
Copy link

+1

@akhil-ga
Copy link
Contributor

akhil-ga commented May 15, 2018

Fixes #1397 #937 #1121 #1895 #1790

@@ -14,12 +14,12 @@ class Root extends Component {
{this.props.children}
<Toast
ref={c => {
if (!Toast.toastInstance) Toast.toastInstance = c;
if (c) Toast.toastInstance = c;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you don't need to check if c is valid. Just do the assignment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look at this @douglasjunior. Does GeekyAnts have a preference on how to handle the case where c is null? @akhil-geekyants @SupriyaKalghatgi

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://reactjs.org/docs/refs-and-the-dom.html#callback-refs

React will call the ref callback with the DOM element when the component mounts, and call it with null when it unmounts. ref callbacks are invoked before componentDidMount or componentDidUpdate lifecycle hooks.

I think if the component has been unmounted, you should not keep the ref. This is memory leak.

@schie
Copy link

schie commented May 18, 2018

@akhil-geekyants @douglasjunior @fredmon3 any updates?

@SupriyaKalghatgi
Copy link
Contributor

This week 💯

@kstolte
Copy link

kstolte commented May 29, 2018

@SupriyaKalghatgi
will this be in the next release?

@SupriyaKalghatgi SupriyaKalghatgi merged commit a903bd7 into GeekyAnts:master Jun 1, 2018
@gogulanareshkw
Copy link

gogulanareshkw commented Oct 25, 2018

"native-base": "^2.8.1",
"react": "^16.4.1",
"react-native": "^0.55.4",
please help me, am getting error as cannot read property 'showActionSheet' of null

to reproduce this issue follow these steps:

actionsheet available in page 1, for the first actionsheet is working
navigate to page 2 from page1, and then page 3 from page2 (page1 -> page2 -> page3)
then goback to page1 and click actionsheet, now getting error

screenshot_2018-10-25-14-59-32

@tapir
Copy link

tapir commented Jan 21, 2019

Having the same issue as @gogulanareshkw.
Trying to show actionsheet twice after selecting an option triggers it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet