Skip to content

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

Merged
domenic merged 3 commits into
mainfrom
state-func
Mar 3, 2021
Merged

Make push()/update() navigate, and change state interface#54
domenic merged 3 commits into
mainfrom
state-func

Conversation

@domenic

@domenic domenic commented Mar 3, 2021

Copy link
Copy Markdown
Collaborator

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

frehner commented Mar 3, 2021

Copy link
Copy Markdown

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?

domenic added 2 commits March 3, 2021 12:04
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

domenic commented Mar 3, 2021

Copy link
Copy Markdown
Collaborator Author

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

domenic commented Mar 3, 2021

Copy link
Copy Markdown
Collaborator Author

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
Copy Markdown
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