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

Add BrowserHistory and BrowserLocation #171

Merged
merged 17 commits into from Nov 29, 2021
Merged

Add BrowserHistory and BrowserLocation #171

merged 17 commits into from Nov 29, 2021

Conversation

futursolo
Copy link
Collaborator

@futursolo futursolo commented Nov 23, 2021

The PR adds an initial BrowserHistory and BrowserLocation implementation that was implemented for yew-router.

Comparing to yew-router's implementation, it strips the following features:

  • typed routes.
  • base name handling.

History leaking

(a.k.a: sometimes it's possible to cause the page to reload, or navigate away from the SPA with certain methods like history.go(0))

After some consideration, I think it ultimately does not matter too much and is kind of hard to prevent.

  • history.go(0) is equivalent to location.reload().
    Which means the user should know what they are doing when they are calling this method with 0.
  • history.back() or history.go(-1) (or history.forward() and history.go(1))
    If the last state is created by history.pushState, then a popstate event will be fired.
    If the last state is on another page, then user will be navigated back to the last page.
    If this is the first state in the history queue, then nothing happens.
    In addition, there's no way to "peek" the next history which means we cannot really know what would happen unless it really happens.

JavaScript version of history does not patch / trying to prevent this either:

https://github.com/remix-run/history/blob/main/packages/history/index.ts#L521

Copy link
Collaborator

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine for the most part. I've left a few comments (some of them apply everywhere but I've noted them at one place)

crates/history/src/any.rs Outdated Show resolved Hide resolved
crates/history/src/history.rs Outdated Show resolved Hide resolved
crates/history/src/history.rs Show resolved Hide resolved
crates/history/src/listener.rs Show resolved Hide resolved
@@ -84,7 +84,7 @@ impl TimeoutFuture {
///
/// # Example
///
/// ```no_run
/// ```compile_fail
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why modify timers here? Let's do it in a separate PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is purely to make CI happy or else CI wouldn't pass.

https://github.com/rustwasm/gloo/runs/4295589107?check_suite_focus=true

Copy link
Collaborator

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for the most part, just a few comments

crates/history/Cargo.toml Outdated Show resolved Hide resolved
crates/history/src/browser.rs Outdated Show resolved Hide resolved
crates/history/src/browser.rs Outdated Show resolved Hide resolved
crates/history/src/listener.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to add this in the previous review...

crates/history/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the references to serde feature which was renamed stuck out to me. Other than that, this looks good to me

crates/history/src/any.rs Show resolved Hide resolved
crates/history/src/browser.rs Show resolved Hide resolved
}

impl Default for BrowserHistory {
fn default() -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw your messages on the Yew discord advising against directly calling it. Should this be pub(crate) function instead of Default implementation? That way, it won't be part of the public API

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn’t recommend it due to it is not a good practice inside components and users should use hooks instead. Not that default implementation itself has any issue.

I think an updated implementation will no longer expose History interface via hooks, so I am fine with default implementation.

In addition, it trips up clippy if you have a new method, but no default implementation.

crates/history/src/history.rs Show resolved Hide resolved
@hamza1311
Copy link
Collaborator

LGTM.

I'm going to go ahead and merge this so it's not a blocking factor for more history-related work in both Gloo and Yew. Should anything needs to be patched up, it can be done in another PR

@hamza1311 hamza1311 merged commit 2245cd9 into rustwasm:master Nov 29, 2021
@hamza1311 hamza1311 mentioned this pull request Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants