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

Make push()/update() navigate, and change state interface #54

Merged
merged 3 commits into from
Mar 3, 2021

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented Mar 3, 2021

Closes #36 by moving from appHistoryEntry.state to getState() and setState() methods. This adds the ability to modify the state of non-current entries as a bonus.

Solves much of #42 by making push() and update() navigate by default, although it does not yet include the initiator idea.

As part of this we simplify update() significantly, since now if you want to change state without performing a navigation, you would use setState(). This makes the URL argument required for it. For symmetry, I removed url from the options for push().

@frehner
Copy link

frehner commented Mar 3, 2021

This PR cleared up a misunderstanding I had, thanks!

And just to clarify, it appears that appHistory.current.setState() doesn't fire the navigate event, correct?

But I assume it would fire the currentchange event?

And should it fire any event if you do setState() on an entry that isn't the current one?

Closes #36 by moving from appHistoryEntry.state to getState() and setState() methods. This adds the ability to modify the state of non-current entries as a bonus.

Solves much of #42 by making push() and update() navigate by default, although it does not yet include the initiator idea.

As part of this we simplify update() significantly, since now if you want to change state without performing a navigation, you would use setState(). This makes the URL argument required for it. For symmetry, I removed url from the options for push().
@domenic
Copy link
Collaborator Author

domenic commented Mar 3, 2021

And just to clarify, it appears that appHistory.current.setState() doesn't fire the navigate event, correct?

Correct.

But I assume it would fire the currentchange event?

And should it fire any event if you do setState() on an entry that isn't the current one?

Good questions. I think it should not fire currentchange, but probably it should have its own per-entry event. Let me add that.

@domenic
Copy link
Collaborator Author

domenic commented Mar 3, 2021

I'm going to merge this now so I can build more on top of it, but any further reviews after the fact are appreciated!

@domenic domenic merged commit 3eee614 into main Mar 3, 2021
@domenic domenic deleted the state-func branch March 3, 2021 17:22
@tbondwilkinson
Copy link
Contributor

I'm not sure about setState().

I think it creates the same problem that we have today with history.pushState() where it's updating the browser about the current state of things AFTER that state has been changed. So on the one hand we're encouraging people to do navigations by storing information in history and responding to those data changes in the navigate event, but this new setState() method is an out - it basically means that there's a workaround to update things in history without ever triggering a navigate event (where we would expect most people's rendering code to be). I think that dilutes the model and is pretty niche.

Further, it means that some part of the application can just clear your state and you don't get any event about it besides currentchange. That also feels bad.

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.

App history state (im)mutability
3 participants