-
Notifications
You must be signed in to change notification settings - Fork 147
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
Conversation
There was a problem hiding this 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)
@@ -84,7 +84,7 @@ impl TimeoutFuture { | |||
/// | |||
/// # Example | |||
/// | |||
/// ```no_run | |||
/// ```compile_fail |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this 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
There was a problem hiding this 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...
There was a problem hiding this 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
} | ||
|
||
impl Default for BrowserHistory { | ||
fn default() -> Self { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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 |
The PR adds an initial
BrowserHistory
andBrowserLocation
implementation that was implemented foryew-router
.Comparing to
yew-router
's implementation, it strips the following features: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 tolocation.reload()
.Which means the user should know what they are doing when they are calling this method with 0.
history.back()
orhistory.go(-1)
(orhistory.forward()
andhistory.go(1)
)If the last state is created by
history.pushState
, then apopstate
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