bug: ion-nav does not remove component in DOM or update state correctly

[daniel-menezes]

Prerequisites

• [X] I have read the Contributing Guidelines.
• [X] I agree to follow the Code of Conduct.
• [X] I have searched for existing issues that already report this problem, without success.

Ionic Framework Version

• [ ] v4.x
• [ ] v5.x
• [X] v6.x
• [ ] Nightly

Current Behavior

When using the IonNav and IonNavLink components in react for routing, going 'forward' creates a new element in the DOM, but going 'backward' does not remove that element. This results in multiple duplicate elements being created when navigating backwards then forwards again (with the current screen being shown in a higher z-index).

As a side effect, it seems than whenever react state is updated inside the IonNav component, a new component is created entirely, rather than updating the state correctly.

Expected Behavior

Would expect IonNav to work correctly in navigating forwards and backwards through IonNavLinks without duplicating elements in the DOM, and correctly updating react state within the components.

Steps to Reproduce

1. Clone: https://github.com/daniel-menezes/ion-nav
2. npm install
3. npm run start
4. Navigate between pages using the given buttons and see the dom renders duplicate components

e.g.

<ion-nav delegate="[object Object]">
  <div class="ion-page ion-page-hidden" style="z-index: 100;" aria-hidden="true">...</div>
  <div class="ion-page can-go-back" style="z-index: 100; display: none;">...</div>
  <div class="ion-page can-go-back" style="z-index: 100; display: none;">...</div>
  <div class="ion-page can-go-back" style="z-index: 100; display: none;">...</div>
  <div class="ion-page can-go-back" style="z-index: 100; display: none;">...</div>
  <div class="ion-page can-go-back" style="z-index: 100; display: none;">...</div>
  ...
  <div class="ion-page can-go-back" style="z-index: 101; display: none;">...</div>
</ion-nav>

I've used a NavComponent as taken from https://github.com/ionic-team/ionic-framework/blob/b2a27e0b623a603b536dc23ab409662219323acb/packages/react/test-app/src/pages/navigation/NavComponent.tsx

Code Reproduction URL

https://github.com/daniel-menezes/ion-nav

Ionic Info

Ionic:

Ionic CLI : 6.20.1 Ionic Framework : @ionic/react 6.2.2

Capacitor:

Capacitor CLI : 4.0.1 @capacitor/android : not installed @capacitor/core : 4.0.1 @capacitor/ios : not installed

Utility:

cordova-res : not installed globally native-run : 1.6.0

System:

NodeJS : v16.13.0 npm : 8.1.0 OS : macOS Monterey

Additional Information

The first video shows the base example of navigating forwards and backwards with IonNav and the DOM creating duplicate components.

https://user-images.githubusercontent.com/61494082/184286345-45d83327-366a-4c85-a34d-9d001ea861f9.mov

The second video shows an example of updating a simple react state counter. The DOM creates a new node with the component in the new state and overlays it underneath the the component with the old state (which can be seen by the z-index.

https://user-images.githubusercontent.com/61494082/184286371-d4e40661-6469-4af4-af12-71d7760d4856.mov

[sean-perkins]

Hello @daniel-menezes thanks for reporting this issue!

Can you test with this dev-build and let me know if you run into any issues?

6.2.3-dev.11660655396.10551559

[daniel-menezes]

Hi @sean-perkins, thanks for the quick turnaround!

The dev build looks good for the navigating as well as updating the state!

One issue I still noticed was that when updating state within an IonNav component, the screen flashes on updates (the DOM seems to change too).

https://user-images.githubusercontent.com/61494082/185014264-41b0ceeb-0f26-45fc-b6e7-cb81bc01836b.mov

Whereas what it should be is that only the state changes on update (shown below on a normal react component).

https://user-images.githubusercontent.com/61494082/185014392-2db21034-91fc-4505-a40f-13d5a18cecc0.mov

I've made a branch with the 6.2.3-dev.11660655396.10551559 build https://github.com/daniel-menezes/ion-nav/tree/6.2.3-dev.11660655396.10551559

[sean-perkins]

Thanks @daniel-menezes! This will take a little extra discovery on my end.

Not initially sure why ion-nav or the inner mounted component would be re-rendered from a state update. The IonNav is a functional component proxy using React Portals for the inner components. It's possible the way that I am mounting the inner component is somehow driving this behavior.

[daniel-menezes]

Hi @sean-perkins, I think I may have figured this out - might have been an issue on how my NavComponent.tsx was written.

Currently I had the count state inside the NavComponent instead of something like PageOne where i think it should be. I refactored out PageOne along with the state, and it looks like everything works! (can check out the latest push to this branch https://github.com/daniel-menezes/ion-nav/tree/6.2.3-dev.11660655396.10551559)

I'm not an expert in react, but this may have fixed it, does this sound like something that would have been causing the issue?

Other than that, the fixes look like it works perfectly now! Is this release something we could expect soon?

Thanks a lot!

[sean-perkins]

@daniel-menezes thanks for digging in further! Yes, the updated commit splitting out the PageOne & state update into a functional component is semantically correct. The prior example was causing a re-render of the parent and all children, leading to the flicker/re-renders.

I'll move that PR from draft to ready for review so the team can take a look 👍

[daniel-menezes]

Hi @sean-perkins thanks for that, really appreciate it!

As a side note, I was having another go at this, and I'm not sure if it's the same issue or related to https://github.com/ionic-team/ionic-framework/pull/25603, but basically I was trying to lift the state up from PageOne to the NavComponent.

I want to do this so that one 'parent' component (NavComponent) has access to the states of children components PageOne, PageTwo, and PageThree, by passing down the state and setState as props (e.g. for a multi step form).

For some reason, using either method, passing down the state as props doesn't work as expected with regular Parent and Child components when lifting up state. Either the entire page component re-renders (as before) or the setCount function passed down becomes undefined.

const root = useCallback(() => <MyComponent first="Sean" last="Perkins" />, []);

<IonNav root={root} />

<IonNav root={MyComponent} rootParams={{ first: "Sean", last: "Perkins" }} />

If you have any ideas on if this is part of an issue, or have any tips on how to do this better that would be greatly appreciated! (I may end up creating a context and provider to achieve this).

[daniel-menezes]

Hi @sean-perkins, do you have any insights as to when the nav fix will be made live? Thanks again

[sean-perkins]

Hello @daniel-menezes there are a few outstanding tasks I need to validate against the dev-build before I am comfortable with the fix going out in a patch release.

Mainly I want to make sure the way that I am removing HTML nodes does not have repercussions when conditionally rendering pages inside IonNav. I also want to verify the state re-rendering challenges you outlined, to see if that is a side effect of my changes or another bug that exists in the IonNav React implementation. If it does exist outside of my changes, we can create a new issue focused on that criteria and minimally merge the fix for unmounting DOM nodes.

[daniel-menezes]

awesome, appreciate it