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

Should we allow setState() separate from update()? #57

Closed
domenic opened this issue Mar 3, 2021 · 2 comments · Fixed by #61
Closed

Should we allow setState() separate from update()? #57

domenic opened this issue Mar 3, 2021 · 2 comments · Fixed by #61
Labels
change A proposed change to the API, not foundational, but deeper than the surface

Comments

@domenic
Copy link
Collaborator

domenic commented Mar 3, 2021

Continuing from @tbondwilkinson's comment at #54 (comment):

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.

I'm sympathetic to these concerns. I think the crucial question is whether we consider a no-URL-update change to the state to be a navigation, or not.

In particular, the discussions in #42 (comment) indicated to me that maybe state-but-not-URL updates are conceptually different. E.g., they would be used for syncing state from the DOM to the state object, and it'd be strange for them to be considered navigations. The design in the explainer after #54 makes it so that update() always causes a full navigation by default, which the app would intercept with navigate. It then expects you to use setState() for other cases.

I may have jumped the gun here though in assuming they are different. If we think that an app will generally want to treat no-URL-update state changes as navigations on the same level as URL-update ones, then we should get rid of setState() and force everything through update().

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.

A late-breaking addition to #54 was the statechange event, so you get that. And to be clear, you don't get currentchange as formulated. Whether currentchange fires or not is tied to a few questions:

@domenic domenic added the change A proposed change to the API, not foundational, but deeper than the surface label Mar 3, 2021
@tbondwilkinson
Copy link
Contributor

I may have jumped the gun here though in assuming they are different. If we think that an app will generally want to treat no-URL-update state changes as navigations on the same level as URL-update ones, then we should get rid of setState() and force everything through update().

There's always the possibility of setState() being a full navigate (with Promise return), who's default behavior is NOT to do a cross-document navigation (much like a hash fragment change via location.hash = '#foo';).

But otherwise, yeah I would say keeping everything going through update is probably a simpler API.

What would be most useful? I still am not sure exactly when people will use currentchange, since navigate is so powerful and per-entry events cover other cases. See #14.

I think currentchange is a simpler API. I think if your page isn't a SPA, this is probably the right API for you to consume history changes (which would mostly be fragment, link clicks, etc.). But I do agree that it's unlikely for a page to split a lot of their logic between the two events.

Do we semantically consider currentchange to mean "the current entry changed to a different entry than it was before" or "something about the current entry changed"? This is very related to #7.

I think it should mean both. Ideally currentchange could replace things like popstate and hashchange wholesale, so I think it should be a superset of those events.

I'm neutral on statechange. I could see it being useful if you only wanted to know when the state changed (i.e. ignoring hashchange events) but didn't want to do the diff checking yourself in currentchange. But maybe that could be folded into currentchange?

@domenic
Copy link
Collaborator Author

domenic commented Mar 4, 2021

Combined with today's offline discussion about the difficulties of maintaining state across multiple processes (especially for non-current entries), I think we should get rid of setState(). I'll work on this change soon.

domenic added a commit that referenced this issue Mar 5, 2021
Partially reverts some of the changes in 3eee614, which upon further consideration were deemed not so good. Closes #57.
domenic added a commit that referenced this issue Mar 5, 2021
Partially reverts some of the changes in 3eee614, which upon further consideration were deemed not so good. Closes #57.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change A proposed change to the API, not foundational, but deeper than the surface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants