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

bug: useLocation may break your app because of mounted pages #27139

Closed
3 tasks done
FernetB opened this issue Apr 6, 2023 · 6 comments
Closed
3 tasks done

bug: useLocation may break your app because of mounted pages #27139

FernetB opened this issue Apr 6, 2023 · 6 comments
Labels

Comments

@FernetB
Copy link

FernetB commented Apr 6, 2023

Prerequisites

Ionic Framework Version

v5.x

Current Behavior

If you have 3 pages A, B and C

You go from A -> B with a stateB and then B -> C with stateC the app may crash because when you are on page C, B is mounted and the stateB that returns the useLocation hook, will have the stateC value because you are no longer on B

This is a silly example of an app crashing with this:

https://codesandbox.io/s/holy-waterfall-t9ofg1?file=/src/pages/UserDetailPage/UserDetailPage.tsx

Expected Behavior

Each page should have their useLocation not muting with the new state.

I know that this is not a bug per se, but a bad interaction between useLocation and Ionic with mounted pages.

But is something that needs to be cared about and or offer a solution, or clariy in the documentation

Steps to Reproduce

Easy example:
1 -Click in any User
2 - Then click on More details button
3 - app break
https://codesandbox.io/s/holy-waterfall-t9ofg1?file=/src/pages/UserDetailPage/UserDetailPage.tsx

Code Reproduction URL

https://codesandbox.io/s/holy-waterfall-t9ofg1?file=/src/pages/UserDetailPage/UserDetailPage.tsx

Ionic Info

Ionic:

Ionic CLI
Ionic Framework : @ionic/react 5.9.1

Capacitor:

Capacitor CLI : 4.1.0
@capacitor/android : 4.1.0
@capacitor/core : 4.1.0
@capacitor/ios : 4.1.0

Utility:

cordova-res : 0.15.1
native-run : 1.7.0

System:

NodeJS : v14.17.0
npm : 6.14.13
OS : Windows 10

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Apr 6, 2023
@liamdebeasi
Copy link
Contributor

Thanks for filing the follow-up issue. So in this case, you're going to need to add an undefined check on person before accessing dni:

dni: {location.state?.person?.dni} <br />

It's certainly not an ideal experience, but it is the intended behavior of inactive pages. We need to keep those pages around in the DOM to a) preserve component state and b) allow for swipe to go back on iOS. This is an experience we are looking to improve, but at the moment this is recommendation for developers.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Apr 6, 2023
@ionitron-bot ionitron-bot bot removed the triage label Apr 6, 2023
@FernetB
Copy link
Author

FernetB commented Apr 10, 2023

Yes, thats fixes the issue, but the main problem is mostly that noone will know of that until they have it.

Would be nice if ionic has a useIonLocation that will be something like useIonLocation<Person> = useLocation<Partial<Person>>

That on top of that you will be able to add some logic if you want.

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Apr 10, 2023
@liamdebeasi
Copy link
Contributor

liamdebeasi commented Apr 10, 2023

Yes, thats fixes the issue, but the main problem is mostly that noone will know of that until they have it.

In that case, it sounds like we should clarify this behavior on the documentation. If you have some thoughts on how we can better explain this, we'd be happy to review a PR. It might be good to place the information in https://github.com/ionic-team/ionic-docs/edit/main/docs/react/lifecycle.md.

@liamdebeasi liamdebeasi added the needs: reply the issue needs a response from the user label Apr 10, 2023
@ionitron-bot ionitron-bot bot removed the triage label Apr 10, 2023
@FernetB
Copy link
Author

FernetB commented Apr 10, 2023

There you have it!

ionic-team/ionic-docs#2894

@liamdebeasi
Copy link
Contributor

Thanks for the contribution! I merged your PR in ionic-team/ionic-docs#2894. I am going to close this, but let me know if you have questions.

@ionitron-bot
Copy link

ionitron-bot bot commented May 13, 2023

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators May 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants