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 appHistory.push() and update() fire the navigate event? #42

Closed
dvoytenko opened this issue Feb 18, 2021 · 18 comments
Closed

Should appHistory.push() and update() fire the navigate event? #42

dvoytenko opened this issue Feb 18, 2021 · 18 comments
Labels
addition A proposed addition which could be added later without impacting the rest of the API

Comments

@dvoytenko
Copy link

Programmatically-initiated same-document navigations initiated via appHistory.push(), appHistory.update(), or their old counterparts history.pushState() and history.replaceState().

The older history.pushState and counterpart did not navigate the document. This is used extensively by routers to reconcile navigations with the history structure when a state might need to be added or updated to provide for back behavior and other application features. There are already several ways to navigate, including location = '...', location.assign(), link.click(), etc. Perhaps the navigation in this API is unnecessary and might be even harmful.

@domenic
Copy link
Collaborator

domenic commented Feb 18, 2021

It depends on what you mean. They definitely navigate the document in the sense of how the spec and implementation sees it. (They're same-document navigations, like a fragment navigation.) So I don't think it's possible to call them harmful in that way, since it's something history.pushState already does.

Could you expand on this a bit more? The intention is for appHistory.push() to navigate the document in the same way history.pushState() does, i.e. be a same-document navigation that updates the URL/state but does not update the Document object. Does that perhaps ameliorate your concern?

@dvoytenko
Copy link
Author

Hmm. I don't think history.pushState does this today. Could you clarify this? No navigation of any kind is triggered via that API as far as I know.

@domenic
Copy link
Collaborator

domenic commented Feb 18, 2021

We discussed this a bit offline. Whether or not pushState is a navigation aside, the main concern is about the navigate event integration.

In particular, today the model people use is: do stuff to update the page, and then use history.pushState() to update the URL. (Or perhaps in the opposite order, but the idea is the same: history.pushState()s job is to update the page's URL to reflect its contents.)

Whereas, the mental model of a pure app history API user is: perform a navigation, either with location.href = or history.pushState() or appHistory.push() or.... Then, in the navigate handler, update the page.

@dvoytenko's is concerned about mixed codebases, e.g. ones migrating from the current model to the app history model. How does their navigate handler deal with the fact that in one part of their code, they're doing updateUI(); history.pushState(), and in other parts of their code, they're just doing appHistory.push() and expecting the navigate handler to call updateUI()?

One solution here, which I think is pretty good, is to be more explicit about the navigation initiator. E.g., inside the navigate event, we could have an event.initiator which is an enum discussing what started the navigation. Then you could write code like

appHistory.addEventListener("navigate", e => {
  // Don't intercept cross-origin navigations; let the browser handle those normally.
  if (!e.sameOrigin) {
    return;
  }

  // Don't intercept fragment navigations.
  if (e.hashChange) {
    return;
  }
  
  // **** Don't handle history.pushState/replaceState() navigations from older parts of the app ****
  // Those parts of the app will do their own UI updating.
  if (e.initiator === "history-pushstate" || e.initiator === "history-replacestate") {
    return;
  }

  if (e.formData) {
    e.respondWith(processFormDataAndUpdateUI(e.formData));
  } else {
    e.respondWith(doSinglePageAppNav(e.destination));
  }
});

@domenic domenic changed the title Should the appHistory.push and update navigate the document? Should appHistory.push() and update() fire the navigate event? Feb 18, 2021
@dvoytenko
Copy link
Author

The navigate event initiator does indeed help with my concern. I'm still not sure where or not a pushState should call the navigate event, but perhaps this can be researched a bit more in the further evaluation.

It's my hope that the vast majority of applications can completely stop using history APIs and instead just rely on the navigate event coupled with all the current navigation mechanisms, including link click, location.assign, window.open. Some of these APIs could be modified to accept history state. E.g. it might make sense to change location.assign(url) to location.assign(url, state). Unfortunately, I don't think location.assign can be polyfilled.

Another option is to also add a boolean flag to history.push APIs to enable/disable navigate event in response.

@domenic
Copy link
Collaborator

domenic commented Feb 22, 2021

The idea of steering applications away from using history APIs entirely is also worth considering. E.g., we wouldn't have appHistory.push() and appHistory.update() at all, but instead something like

location.navigate(url, { state, replace, navigateInfo });

@dvoytenko
Copy link
Author

I'd really like to move in that direction. I'd only keep appHistory.push/update for some super advanced and unusual cases. IMHO the history is used directly by apps today for many wrong reasons.

@domenic
Copy link
Collaborator

domenic commented Feb 24, 2021

So in that direction I think the relevant questions are:

  1. Should the existing history.pushState() and history.replaceState() APIs fire the navigate event?

  2. What should the new navigation APIs (currently appHistory.push() and appHistory.update()) do by default, if the navigate handler doesn't intercept them? Should they do a cross-document or fragment navigation based on the URL given, like location.assign()/location.replace(), or should they only update the current URL and state, like history.pushState()/history.replaceState()?

    • Is the answer different for update() vs. push()? It seems like updating the current entry's URL, or especially state, without performing a navigation, still has use cases even in a navigate-centric world.
  3. Where should the new APIs live? E.g. the above talks about moving them to location.navigate() instead of them living on appHistory.

For (1), I'm pretty sold they should fire the navigate event, with some initiator property to let you filter them out as necessary. Giving people a holistic view of all navigations (in the browser sense, i.e. things which change the URL/state/etc.) seems important for a variety of use cases.

For (2) and (3), I like the idea of converting appHistory.push() into a new location.navigate() with full-navigation default behavior. This also came up in talking with @annevk; he mentioned that people have wanted a better method for performing navigations for some time, which exposes some of the relevant primitives (e.g. replace vs. new entry, form data, probably some other stuff) in a coherent way. I'm less sure about appHistory.update(); as I mentioned above that still seems useful...

@frehner
Copy link

frehner commented Feb 24, 2021

Agreed on 1 - if they don't fire the navigate event, then that would just require developers to monkeypatch those calls instead to know if they're being called or not.

I'm not sure I agree on changing appHistory.push() to location.navigate() - it seems counterintuitive/confusing to add a new API (appHistory) with event listeners, entries, current, update(), etc but then say "and the way you interact with it is through location.navigate()."

Perhaps I'm not fully understanding the added value of changing it to be off on its own, when everything else is still part of appHistory though?

@domenic
Copy link
Collaborator

domenic commented Feb 24, 2021

Yeah, I'm not sure I explained it that well...

Another way to think of it is just deleting appHistory.push() and saying that you should use location.assign()/location.replace()/the various location setters. That's almost equivalent, except then you lose out on the ability to update the app history state, or send navigateInfo. So we add location.navigate() as a method that is like a better version of location.assign() and location.replace(). Or maybe we could just add a new parameter to those existing methods.

Recall that the bigger issue being discussed here is the two models in #42 (comment). Removing appHistory.push() is designed to nudge people toward the app history model, of intercepting navigate events.

@frehner
Copy link

frehner commented Feb 24, 2021

Ah, so the added value is essentially: it has a better chance to force people to rethink how their code flows with appHistory vs the current flow? Otherwise, they're functionally the same?

Though it doesn't guarantee it (I could see devs writing articles that still just say "change history.push() to location.navigate() and you're done")

Hopefully I'm understanding you correctly - forgive me for my slowness in understanding. 🙂

@domenic
Copy link
Collaborator

domenic commented Feb 24, 2021

No worries, I'm happy to walk through it more.

I could see devs writing articles that still just say "change history.push() to location.navigate() and you're done"

The difference is in my point (2). In particular, history.pushState()'s default behavior is to just update the URL and/or state. Whereas location.navigate()'s default behavior is like that of location.assign() or location.replace(): it will perform a full cross-document navigation (or scroll to a fragment), unless intercepted by navigate.

So just changing history.pushState() to location.navigate() will cause big behavior changes, unless you have an appropriate navigate handler intercepting cross-document navigations. Thus the nudge.

@frehner
Copy link

frehner commented Feb 24, 2021

Ah, thank you for your patience in explaining it to me; I think I understand now.

So a SPA router would just need to add a navigate handler that would essentially be

appHistory.addEventListener('navigate', (event) => {
  if(event.sameOrigin) {
    reRenderSPA();
    event.preventDefault();
  }
)

(maybe the router could add an additional check in navigateInfo to allow a developer to force a full cross-document navigation even if it's the same origin? Or would you maybe want to add an explicit option for this?)

But I do now see the value of having a single api that can fire both cross-document and also SPA navigations.

It seems like it would even allow the developer to not worry about the environment that they're in; they could call location.navigate() and the environment determines how to handle that.

That is, if I've finally understood you correctly 🙂

@domenic
Copy link
Collaborator

domenic commented Feb 24, 2021

Yeah, that's exactly it!

To be clear, I'm not 100% sure we should go this route, but it makes a good deal of sense...

@annevk
Copy link

annevk commented Feb 25, 2021

(Not extending Location further and co-locating navigation with history might also be attractive, given the complexities of the Location object.)

@tbondwilkinson
Copy link
Contributor

tbondwilkinson commented Feb 25, 2021

Should the existing history.pushState() and history.replaceState() APIs fire the navigate event?

+1 to yes

What should the new navigation APIs (currently appHistory.push() and appHistory.update()) do by default, if the navigate handler doesn't intercept them? Should they do a cross-document or fragment navigation based on the URL given, like location.assign()/location.replace(), or should they only update the current URL and state, like history.pushState()/history.replaceState()?

I think I'm starting to feel like a full cross-document navigation for push/update makes sense. It's always been pretty bizarre that history.pushState() does NOT fallback to requesting the URL from the server. And to the current behavior as history.pushState it's trivial to install a navigation handler that just preventsDefault on anything that comes from appHistory that shares the same URL path, etc.

Is the answer different for update() vs. push()? It seems like updating the current entry's URL, or especially state, without performing a navigation, still has use cases even in a navigate-centric world.

I think if you're updating any part of the URL, it would be a cross-document navigation. If you update the hash or do not update the URL (just state) it would NOT perform a corss-document navigation because the "resource" is assumed to be the same. Maybe? I believe this is how Location works today re:URL changes.

Where should the new APIs live? E.g. the above talks about moving them to location.navigate() instead of them living on appHistory.

I feel pretty strongly that we shouldn't add this to Location unless someone could sell me on the narrative that it would be clearer. I think ideally, in a modern application, you wouldn't need to use the Location Object at all, and could purely use appHistory, so it would be a regression to me if a modern application had to use Location for some APIs but appHistory for others. That implies though that anything you can do with Location you can do with appHistory, and there's a few things that's not true for, like reload(). What do we think about that?

@domenic domenic added the foundational Foundational open questions whose resolution may change the model dramatically label Feb 25, 2021
@domenic
Copy link
Collaborator

domenic commented Feb 25, 2021

I feel like we're converging on push and update, nice.

Regarding where to add it, I don't have strong opinions. I think @dvoytenko's perspective is something like "location should be used for navigating, appHistory should be used for history" to mirror the current location/history split. But I can also see the point of view that these things are very related to each other and just colocating everything might be better. E.g., appHistory.back() does a navigation, and appHistory.addEventListener("navigate", ...) is about navigation, so...

@frehner
Copy link

frehner commented Mar 1, 2021

Regarding where to add it

I would vote for everything being added to appHistory, but that's just a personal preference.


On a somewhat related note: I think I may have missed this in the discussion about the navigate method (and I'm not sure if this issue is the best place to discuss it or not; maybe a new issue should be opened to talk about it?) but how would you intercept a cross-document navigation (for SPA routing) while still allowing the navigation to succeed? event.preventDefault() currently causes the navigation to fail/rollback, right? Would there need to be a different way to signal that the cross-document navigation was cancelled but was successfully converted to a same-document (SPA) navigation? [edit] or perhaps it fits into #47

But it's also possible I missed that in our conversation above 😊

domenic added a commit that referenced this issue 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().
domenic added a commit that referenced this issue 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().
domenic added a commit that referenced this issue 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 well as a statechange event, 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 added addition A proposed addition which could be added later without impacting the rest of the API and removed foundational Foundational open questions whose resolution may change the model dramatically labels Mar 16, 2021
@domenic
Copy link
Collaborator

domenic commented May 20, 2021

I think we're pretty solid on this for now, with #115 tracking the remaining issue.

@domenic domenic closed this as completed May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition A proposed addition which could be added later without impacting the rest of the API
Projects
None yet
Development

No branches or pull requests

5 participants