Use navigate instead of resolver

[ilteoood]

Changes
In the routes, use the Navigate element instead of redirect resolver

Issues
Fixes jellyfin/jellyfin-tizen#279

[dmitrylyzo]

FYI, we backport from the release branch to master, so other PR may be closed.

Does this fix the Tizen issue? I mean, have you tested this?

[ilteoood]

Yes, yesterday before opening the pr. My patched version is the one I'm running on the tv now.

[thornbill]

I'm curious what is the underlying issue with using redirect? Is it using something internally that we fail to polyfill?

[ilteoood]

I'm curious what is the underlying issue with using redirect? Is it using something internally that we fail to polyfill?

It seems to be an odd internal behavior of the router: on tizen, instead of looking for the resolver, it checks just for the element property and then throws the error.
Additionally, looking at the official documentation seems that redirect should be used when handling http calls. In other scenarios, the Navigate is the way to go

[dmitrylyzo]

Previously (before react-router) on Tizen 2.x (WebKit) we had an issue where the popstate event was triggered on page load, making the site think we had performed a back action.
Could it be related?

[ilteoood]

I don't think so, this is more a library specific implementation rather than issue with the js engine itself

[ilteoood]

Here is the piece of code that is triggering it: https://github.com/remix-run/react-router/blob/0f2b167e9b862d5e6b3425438787c8e7b39197f4/packages/react-router/lib/hooks.tsx#L450

As you can see only element, component and lazy are checked. No loader

[ilteoood]

Hi @thornbill, is this ready to be merged or do I need to do something more?

[ilteoood]

Ping @thornbill

[thornbill]

You really haven't explained why this is an issue for Tizen. The code you link to appears to only print a developer warning and looking at the message

This means it will render an with a null value by default resulting in an "empty" page.

it describes the behavior we want to happen. Also if you look at the react-router examples, you can see they have an authentication example that our code is based on (a loader returning a redirect).

[ilteoood]

@thornbill
The issue is explained in the original issue jellyfin/jellyfin-tizen#279.
It is an issue because it doesn't load the login screen.
Additionally, on the exaple you pasted I don't see a redirect but just an object instead of an http call. In fact, according to the official doc, it should be used to provide data to the route: https://reactrouter.com/en/main/route/loader, which is not our scenario.

[thornbill]

My question is why is this only an issue for Tizen. If the problem was that react router doesn't support what we are doing for with the redirect then it wouldn't work anywhere.

[ilteoood]

Honestly speaking I don't know. The Tizen version I'm using is very old and the peculiarity of its engine implementation could have some bugs that can be hardly reproduced elsewhere. This would not be a new thing, since even in this repo there are some patches just for this platform.
e.g. https://github.com/jellyfin/jellyfin-web/blob/master/src/legacy/patchHeaders.js

[ilteoood]

BTW, are we seriously spending weeks discussing on a PR of 2 lines of code (repeated 2 times), that changes stuff following the library's best practice?
Version 10.9.9 has been released today and I'll need to manually patch it again to make it work on my TV.

[ilteoood]

Looking it a bit deeper it could be that my tizen version doesn't fully support the header or the response classes to do redirects.
Here is the implementation I checked: https://github.com/remix-run/react-router/blob/55c18bfc8e20417204cce5ecda88fe03520ad257/packages/router/utils.ts#L1615

I'm supposing it because jellyfin already has a patch for the headers constructor for tizen. Unfortunately I'm not 100 sure about it because for my tizen version there isn't any emulator and test each time on the TV is extremely painful.

[ilteoood]

@thornbill Another user has the same issue: jellyfin/jellyfin-tizen#279 (comment)

Can we consider merging this?

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud