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

Remove router state #2170

Closed
wants to merge 1 commit into from
Closed

Conversation

voidpumpkin
Copy link
Member

Description

During #2118 router state was reintroduced, even though there were previous efforts to remove it by @hamza1311
This PR removes the re-added state

Checklist

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

@voidpumpkin voidpumpkin added the A-yew-router Area: The yew-router crate label Nov 21, 2021
@voidpumpkin
Copy link
Member Author

@futursolo Sorry for my bad review on that PR :D

@ranile
Copy link
Member

ranile commented Nov 21, 2021

@futursolo wants to move history/location access to Gloo (rustwasm/gloo#166). I think it'd be better swap out the entire code with Gloo, instead of removing parts of it right now

@voidpumpkin
Copy link
Member Author

@futursolo wants to move history/location access to Gloo (rustwasm/gloo#166). I think it'd be better swap out the entire code with Gloo, instead of removing parts of it right now

I see, well I can close this PR, we can always reopen if needed

@futursolo
Copy link
Member

May I ask what's the rationale of removing state from history?

@voidpumpkin
Copy link
Member Author

@futursolo It adds more complexity for router related components like Link and Redirect. While it is possible to pass state by other means in yew.
I'm not inherently against it, its just sometimes less is more.

@futursolo
Copy link
Member

futursolo commented Nov 22, 2021

It adds more complexity for router related components like Link and Redirect.

I am not sure I understand this?
This PR does not seem to remove anything from those components.

The rationale of having a state on the history is as following, for example, if you want to preserve the scrolling position during navigations:

history.push_with_state(Path::A, State { scroll_y: 123 }); // id: 0
history.push_with_state(Path::B, State { scroll_y: 456 }); // id: 1
history.push_with_state(Path::A, State { scroll_y: 133 }); // id: 2

If one wants to implement this without access to history-based state, it will be very difficult as there will be no other way to reliably figure out which page the user is currently on (in terms of session history position, like, how to distinguish between id 0 and 2). JavaScript version of history provides a unique key to distinguish between navigations, but as far as I know, it is also implemented using window.history.state.

(FWIW, this GitHub page uses history-based state.)

@voidpumpkin voidpumpkin deleted the remove-state branch November 22, 2021 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew-router Area: The yew-router crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants