gloo-history based `yew-router` #2239

futursolo · 2021-12-05T10:54:42Z

Description

This pull request consists of the following changes:

Histories are now provided by gloo-history.
HashRouter for hash-based routing.
Navigator API which supersedes the History API and shields location() & listen() access (users should use hooks & RouterScopeExt instead).
Navigator hooks and contexts only update when History is swapped with a different one or basename changes.
Lints on hashs in routes and relative paths in routes.

Checklist

I have run cargo make pr-flow
I have reviewed my own code
I have added tests

futursolo · 2021-12-05T10:55:42Z

This PR is still blocked by: rustwasm/gloo#178

So I am using a branch that resides on my fork of gloo.

But other than that, this PR is good to go.

I will update this PR when gloo-history is released.

github-actions · 2021-12-05T11:00:54Z

Visit the preview URL for this PR (updated for commit 27beca5):

https://yew-rs--pr2239-gloo-history-quxda239.web.app

_{(expires Sat, 25 Dec 2021 13:14:16 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

# Conflicts: # packages/yew-router/src/components/link.rs

futursolo · 2021-12-05T15:07:46Z

I have updated this PR to use the latest version of gloo.

Also reverted most of #2214, sorry.

Follpvosten · 2021-12-05T15:59:48Z

Can confirm that this works for me (and would fix my main blocker for migration to yew 0.19, which is hash routing).

A minor thing I noticed (which is not a deal breaker for me) is that <Link>s don't have a hash in the href; I know this might be hard to fix and it's not really of much concern to me personally (because the component catches the onclick event anyways), but it's something that caught my eye.

Follpvosten · 2021-12-05T21:42:44Z

Oh, I discovered another issue: Reloading the page via Ctrl-R always causes a not found. Still not a dealbreaker for me though.

futursolo · 2021-12-06T10:09:25Z

Oh, I discovered another issue: Reloading the page via Ctrl-R always causes a not found. Still not a dealbreaker for me though.

I cannot reproduce this by swapping BrowserRouter with HashRouter in the router example.

Could you please provide a reproducible example?

Follpvosten · 2021-12-06T13:51:53Z

Could you please provide a reproducible example?

I will look into it. It might have to do with a JS library I use - namely, keycloak's JS library, which I mostly treat as a black box that handles authentication for me. It does this thing where it redirects to a different page and appends a bunch of query parameters; it removes them again when it's done with its things, but maybe this confuses the HashRouter somehow (which I only create after all the auth stuff is handled, but who knows).

Then again, as I said, this is not a big issue for me; having my users always start on the default page is not really an issue for me. So I'm likely to postpone this until I have some free time.

hamza1311

Looks good for the most part, just a few comments. Also, please fix the conflicts.

packages/yew-router/src/history.rs

packages/yew-router/src/navigator.rs

packages/yew-router/src/router.rs

…tory # Conflicts: # packages/yew-router/Cargo.toml # website/docs/concepts/router.md

hamza1311

LGTM.

If any issues come up, those can be dealt with in future PRs

voidpumpkin

Minor comments, would be good to resolve them on this PR so we dont forget in the future

Also Im seeing that the router page is becoming overloaded, we should split it in the future or it could possible be done in this PR if you want to

website/docs/concepts/router.md

voidpumpkin · 2021-12-15T17:41:08Z

website/docs/concepts/router.md

@@ -45,7 +45,7 @@ matched, the router navigates to the path with `not_found` attribute. If no rout
 a message is logged to console stating that no route was matched.

 Finally, you need to register the `<Router />` component as a context.
-`<Router />` provides session history information to its children.
+`<Router />` provides location information and navigator to its children.

 When using `yew-router` in browser environment, `<BrowserRouter />` is recommended.


This bit could be expanded about BrowserRouter vs HashRouter

This is a deliberate choice that I made.
My rationale is that we should document the good design in the documentation.

If you really want to know everything, you should look at the API Reference (docs.rs).

It's almost never a good idea to use hash based routing and it's really a last resort when you have ran out of options.
(Poor SEO, Query string is emulated (which is also the cause of problem @Follpvosten is having), Collides with anchors, etc.)

@futursolo Even though I agree with you, i am afraid of always having to answer this question in discord.
Perhaps we could just emphasize more that there are others. like the suggestion in other comment

website/docs/concepts/router.md

Follpvosten · 2021-12-16T12:05:17Z

Update on my problem: I've verified that it works if I only create my HashRouter once auth has been fully handled. I still can't provide a reproducing example, but since this has fixed the issue for me, I can't really justify investing more time.

I'm assuming it's because there is a Query on the route at the start, somehow causing a NotFound which is handled and redirects to the default page once my Switch is created (which is the part that was previously only done once auth has been fully handled).

I think while writing this, I realized why the query causes a NotFound: Likely because it's after the hash route, it's interpreted to be a part of the route, not a separate thing.

This is how the URL looks while Keycloak is doing its thing:

EDIT: So, looking at that URL, it's pretty clear that Keycloak is at fault here, because that's not really a valid query as it doesn't start with ?. I'm keeping my solution for now and will look into reporting that at Keycloak.

Co-authored-by: Julius Lungys <32368314+voidpumpkin@users.noreply.github.com>

website/docs/concepts/router.md

voidpumpkin

Perfect, just one comment regarding the title of the PR
And it can be done by whoever ends up merging this, probaly you @hamza1311 from the looks of it.
Since we use commit text when generating the changelog, the commit should have mentions that it adds basename and Hash router.
Title is important because default commit name when merging gets generated based on title.

We could just make it like Add HashRouter, basename and use gloo-history

voidpumpkin · 2021-12-18T13:38:07Z

or i can just go ahead and do it with the auto merge :D

futursolo added 5 commits December 5, 2021 13:22

Migrate to gloo-history.

a9bf4dd

Fix docs wording.

db01465

Add basename handling to navigator.

11992c6

Fix basename handling.

8ebb3d5

Add lints and tests.

98b2abd

futursolo added 3 commits December 5, 2021 22:15

Fix wording.

a192ec9

Merge remote-tracking branch 'origin/master' into gloo-history

3dadfa1

# Conflicts: # packages/yew-router/src/components/link.rs

Fix docs.

ded1c3c

futursolo added 2 commits December 6, 2021 00:23

Fix pr-flow.

6158413

Fix some documentation.

54695e3

Add Navigator Kind.

8ddfdf0

hamza1311 reviewed Dec 13, 2021

View reviewed changes

packages/yew-router/src/history.rs Outdated Show resolved Hide resolved

packages/yew-router/src/navigator.rs Show resolved Hide resolved

packages/yew-router/src/navigator.rs Show resolved Hide resolved

packages/yew-router/src/router.rs Show resolved Hide resolved

futursolo added 3 commits December 14, 2021 15:01

Merge commit 'cf0f59af559bb219c4658e2d98b747b942ffee50' into gloo-his…

b0f2ad5

…tory # Conflicts: # packages/yew-router/Cargo.toml # website/docs/concepts/router.md

Remove history.rs

6843575

Add documentation for navigator & Concrete Router Props.

4cb0323

hamza1311 previously approved these changes Dec 14, 2021

View reviewed changes

hamza1311 requested review from voidpumpkin and siku2 December 14, 2021 14:27

voidpumpkin added A-yew-router Area: The yew-router crate A-yew-router-macro Area: The yew-router-macro crate labels Dec 15, 2021

voidpumpkin requested changes Dec 15, 2021

View reviewed changes

Update website/docs/concepts/router.md

78798c4

Co-authored-by: Julius Lungys <32368314+voidpumpkin@users.noreply.github.com>

futursolo dismissed hamza1311’s stale review via 78798c4 December 18, 2021 05:10

Add docs about basename.

94332d7

voidpumpkin reviewed Dec 18, 2021

View reviewed changes

website/docs/concepts/router.md Outdated Show resolved Hide resolved

futursolo added 2 commits December 18, 2021 22:06

Update documentation.

d591e65

Fix docs.

27beca5

voidpumpkin approved these changes Dec 18, 2021

View reviewed changes

voidpumpkin enabled auto-merge (squash) December 18, 2021 13:38

futursolo requested a review from hamza1311 December 19, 2021 13:20

hamza1311 approved these changes Dec 19, 2021

View reviewed changes

voidpumpkin merged commit d8ec501 into yewstack:master Dec 19, 2021

futursolo mentioned this pull request Dec 22, 2021

yew_router should provide an ergonomic way to access the fragment from the URL #1858

Closed

futursolo deleted the gloo-history branch January 27, 2022 13:59

SionoiS mentioned this pull request Oct 5, 2022

Dependency Hell Defluencer/social_defluencer#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gloo-history based `yew-router` #2239

gloo-history based `yew-router` #2239

futursolo commented Dec 5, 2021 •

edited

futursolo commented Dec 5, 2021 •

edited

github-actions bot commented Dec 5, 2021 •

edited

futursolo commented Dec 5, 2021

Follpvosten commented Dec 5, 2021

Follpvosten commented Dec 5, 2021

futursolo commented Dec 6, 2021

Follpvosten commented Dec 6, 2021

hamza1311 left a comment

hamza1311 left a comment

voidpumpkin left a comment •

edited

voidpumpkin Dec 15, 2021

futursolo Dec 18, 2021

voidpumpkin Dec 18, 2021

Follpvosten commented Dec 16, 2021 •

edited

voidpumpkin left a comment •

edited

voidpumpkin commented Dec 18, 2021

gloo-history based yew-router #2239

gloo-history based yew-router #2239

Conversation

futursolo commented Dec 5, 2021 • edited

Description

Checklist

futursolo commented Dec 5, 2021 • edited

github-actions bot commented Dec 5, 2021 • edited

futursolo commented Dec 5, 2021

Follpvosten commented Dec 5, 2021

Follpvosten commented Dec 5, 2021

futursolo commented Dec 6, 2021

Follpvosten commented Dec 6, 2021

hamza1311 left a comment

Choose a reason for hiding this comment

hamza1311 left a comment

Choose a reason for hiding this comment

voidpumpkin left a comment • edited

Choose a reason for hiding this comment

voidpumpkin Dec 15, 2021

Choose a reason for hiding this comment

futursolo Dec 18, 2021

Choose a reason for hiding this comment

voidpumpkin Dec 18, 2021

Choose a reason for hiding this comment

Follpvosten commented Dec 16, 2021 • edited

voidpumpkin left a comment • edited

Choose a reason for hiding this comment

voidpumpkin commented Dec 18, 2021

gloo-history based `yew-router` #2239

gloo-history based `yew-router` #2239

futursolo commented Dec 5, 2021 •

edited

futursolo commented Dec 5, 2021 •

edited

github-actions bot commented Dec 5, 2021 •

edited

voidpumpkin left a comment •

edited

Follpvosten commented Dec 16, 2021 •

edited

voidpumpkin left a comment •

edited