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 Router renders white page with Redirect #28838

Closed
3 tasks done
lorenzo-bonatti opened this issue Jan 16, 2024 · 6 comments · Fixed by #28961
Closed
3 tasks done

bug: React Router renders white page with Redirect #28838

lorenzo-bonatti opened this issue Jan 16, 2024 · 6 comments · Fixed by #28961
Labels
package: react @ionic/react package type: bug a confirmed bug report

Comments

@lorenzo-bonatti
Copy link

Prerequisites

Ionic Framework Version

v7.x

Current Behavior

From the version v7.5.1, there is a bug with the Redirect component.

Let's suppose that we want to manage our routes with authentication and that the routes redirect to the login or home page depending on a context (ex. Redux state)

A possible route definition can be like this:

<IonReactRouter>
    <IonRouterOutlet>
        <Route path="/login" exact render={() => !auth.isAuth ? <Login/> : <Redirect to="/"/>}/>
        <Route path="/" exact render={() => auth.isAuth ? <Home/> : <Redirect to="/login"/>}/>
        <Route path="/about" exact render={() => auth.isAuth ? <About/> : <Redirect to="/login"/>}/>
    </IonRouterOutlet>
</IonReactRouter>

If I'm on the login page and the state changes, the component Redirect works correctly and it redirects me to the home page.
The problem occurs if I come back to the login page (for example with the logout): the page will be blank.

Expected Behavior

The page renders correctly as in previous versions. (<= v7.5.0)

Steps to Reproduce

  1. Render the first page;
  2. Change the global state to render the Redirect component;
  3. Return to the first page with another state change;

Code Reproduction URL

https://github.com/lorenzo-bonatti/my-ionic-app

Ionic Info

Ionic:

   Ionic CLI       : 7.2.0
   Ionic Framework : @ionic/react 7.6.4

Utility:

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

System:

   NodeJS : v20.11.0 (/home/lorenzo/.asdf/installs/nodejs/20.11.0/bin/node)
   npm    : 10.2.4
   OS     : Linux 6.5

Additional Information

No response

@ionitron-bot ionitron-bot bot added the triage label Jan 16, 2024
@sean-perkins sean-perkins self-assigned this Jan 17, 2024
@sean-perkins
Copy link
Contributor

Hello @lorenzo-bonatti thanks for reporting this issue!

I can confirm this behavior regressed in v7.5.1 from this change: #28316 (specifically the change to PageManager).

I will need to do additional discovery to see why the Redirect component is effected by this change.

@sean-perkins sean-perkins added the package: react @ionic/react package label Jan 17, 2024
@ionitron-bot ionitron-bot bot removed the triage label Jan 17, 2024
@sean-perkins sean-perkins added the type: bug a confirmed bug report label Jan 17, 2024
@sean-perkins sean-perkins removed their assignment Jan 17, 2024
@sean-perkins
Copy link
Contributor

Hello @lorenzo-bonatti, I've done some additional investigation and I think we can leave the removal of the event listener when the element node is detached and garbage collected.

If you would to test with this dev-build and let me know if you experience any unexpected issues:

npm install @ionic/react@7.6.7-dev.11706567011.11e782a9 @ionic/react-router@7.6.7-dev.11706567011.11e782a9

You will need to delete the node_modules/.vite directory to avoid the dependency pre-bundling feature.

Thanks!

@lorenzo-bonatti
Copy link
Author

Hello @sean-perkins

I tried your code versions with my my-ionic-app project, and it works properly.

Can I use this version for my production project, or is it better wait for the release?

Thanks for you support!

@sean-perkins
Copy link
Contributor

Hello @lorenzo-bonatti, thank for verifying the dev-build!

I would recommend staying with your current version (< 7.5.1) in a production environment. Once this fix is officially reviewed and merged by the team, it would be acceptable to use in production.

I want to investigate a few more techniques before moving forward with removing the event listener: 1e782a9.

@aeharding
Copy link
Contributor

FWIW, I ran into this bug and I've been testing your patch @sean-perkins, and it's working well so far!

aeharding added a commit to aeharding/voyager that referenced this issue Feb 2, 2024
aeharding added a commit to aeharding/voyager that referenced this issue Feb 2, 2024
github-merge-queue bot pushed a commit that referenced this issue Feb 6, 2024
Issue number: resolves #28838

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

In #28316 we resolved a longstanding misconfiguration where event
listeners being added to the page were not removed. This was due to
incorrect usage of `.bind` creating a new instance of the callback
functions.

By removing the event listener for `ionViewDidLeave`, before the
component has actually unmounted in react, resulted in the registered
destroy callback to not fire:
https://github.com/ionic-team/ionic-framework/blob/51c729eafc3b290a5daddf7f0ccffd0a2a9fe2aa/packages/react/src/contexts/IonLifeCycleContext.tsx#L208-L216
and
https://github.com/ionic-team/ionic-framework/blob/51c729eafc3b290a5daddf7f0ccffd0a2a9fe2aa/packages/react/src/routing/ViewLifeCycleManager.tsx#L21-L32

This resulted in a scenario that using a `Redirect` could cause the
wrong view to be unmounted (the entering view) and leave the user on an
empty screen.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- `ionViewDidEnter` event listener *is not* removed while the component
is unmounting. The browser will naturally remove the event listener when
the element node is detached from the DOM.
- Users are no longer presented with a white screen after clicking a
route that uses a redirect.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!--
  If this introduces a breaking change:
1. Describe the impact and migration path for existing applications
below.
  2. Update the BREAKING.md file with the breaking change.
3. Add "BREAKING CHANGE: [...]" to the commit description when merging.
See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#footer
for more information.
-->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

Dev-build: `7.6.7-dev.11706567011.11e782a9`
Copy link

ionitron-bot bot commented Mar 8, 2024

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 Mar 8, 2024
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