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: React overlay hooks (useIon*) are not fully memorised #23741

Closed
4 of 6 tasks
FWeinb opened this issue Aug 8, 2021 · 5 comments · Fixed by #24010
Closed
4 of 6 tasks

bug: React overlay hooks (useIon*) are not fully memorised #23741

FWeinb opened this issue Aug 8, 2021 · 5 comments · Fixed by #24010
Labels
package: react @ionic/react package type: bug a confirmed bug report

Comments

@FWeinb
Copy link
Contributor

FWeinb commented Aug 8, 2021

Prequisites

Ionic Framework Version

  • v4.x
  • v5.x
  • v6.x

Current Behavior

Using the new overlay hooks feels great and is a nice improvement in my opinion. When using them I noticed that for useIon{Toast|Alert|Popover|...} the results aren't memorised and change each render. This will cause a great deal of overhead when the present/dismiss methods will be dependencies of other useEffects.

You can see a demo of this behaviour here

Expected Behavior

All the useIon* hooks should memorised the returned present/dismiss to match developers expectation.

Steps to Reproduce

  1. Use any of the new overlay hooks e.g. useIonToast
  2. Make it a dependency of React.useEffect(..., [presentToast])
  3. See that this effect is reevaluated each time the component is rendered.

Code Reproduction URL

https://codesandbox.io/s/ionic-hooks-memorisation-5o4yc?file=/src/App.tsx

Ionic Info

No response

Additional Information

I tracked this issue down to useController. Adding some React.useCallback here

const present = async (options: OptionsType & HookOverlayOptions) => {

and here
const dismiss = async () => {

and a React.useMemo
return {
present,
dismiss,
};

will ensure that the controller is memorised. Looking at the other useIon* hooks should be straight forward. Fixing useIonToast would need a React.useCallback here
function present(messageOrOptions: string | ToastOptions & HookOverlayOptions, duration?: number) {

If you think this is something that should be implemented I can create a PR for these changes.

@ionitron-bot ionitron-bot bot added the triage label Aug 8, 2021
@willmartian
Copy link
Contributor

Hey @FWeinb, thanks for the issue and the useful code reproduction!

I agree that this is unwanted behavior and I think your proposed solution is in the right direction. If you could create a PR for this, that would be very helpful. 😄

@willmartian willmartian added package: react @ionic/react package type: bug a confirmed bug report labels Aug 10, 2021
@ionitron-bot ionitron-bot bot removed the triage label Aug 10, 2021
@FWeinb
Copy link
Contributor Author

FWeinb commented Aug 15, 2021

Closing this because I submitted a PR fixing this.

@FWeinb FWeinb closed this as completed Aug 15, 2021
@liamdebeasi
Copy link
Contributor

Thanks for the PR! I am going to keep this issue open until the PR gets merged.

@willmartian
Copy link
Contributor

Thanks for the contribution on this issue @FWeinb! The fix for this issue has been merged and will be available in the next release of Ionic Framework.

@ionitron-bot
Copy link

ionitron-bot bot commented Nov 4, 2021

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 Nov 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: react @ionic/react package type: bug a confirmed bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants