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

Add appHistory.transition with first-class rollback #90

Merged
merged 5 commits into from
Apr 5, 2021
Merged

Conversation

domenic
Copy link
Collaborator

@domenic domenic commented Apr 2, 2021

This gives insight into any "ongoing" navigation, i.e. a navigation that hasn't yet reached navigationsuccess/navigationerror because the promise passed to respondWith() has not yet settled.

Additionally:

  • Removes the finished property from every AppHistoryEntry, which is nice because it only ever really made sense on the current entry; it was about the transition. Instead, the presence or nullness of appHistory.transition can be used.
  • Adds a type property to AppHistoryNavigateEvent since we're going to have it on appHistory.transition anyway so having it earlier (before respondWith() is called) seems very reasonable.

Closes #86. Closes #11 by adding the appHistory.transition.finished promise.

Helps with #41 as you can retrieve data from the replaced entry, at least during the transition, with appHistory.transition.previous.

Probably helps with #14 (although currentchange needs a bit of an overhaul) since appHistory.transition has the sort of useful information discussed there, such as the previous entry or the type of navigation.


I omitted the initiated timestamp for now because: (a) it's still unclear what we're doing with time measurement in general, per #33 and #59; (b) the "transition" was initiated when you called respondWith(), and that seems a bit less useful than the time that the user clicked their mouse or the time when you did location.href or whatever. Usually they'll be close, but especially with input lag not always...

Potential bikeshed points I'd be happy to get feedback on:

  • appHistory.transition vs. appHistory.ongoing vs. appHistory.currentNavigation vs. ...
  • "traversal" vs. "go"
  • appHistory.transition.previous vs. appHistory.transition.source vs. ...

This gives insight into any "ongoing" navigation, i.e. a navigation that hasn't yet reached navigationsuccess/navigationerror because the promise passed to respondWith() has not yet settled.

Additionally:

* Removes the finished property from every AppHistoryEntry, which is nice because it only ever really made sense on the current entry; it was about the transition. Instead, the presence or nullness of appHistory.transition can be used.
* Adds a type property to AppHistoryNavigateEvent since we're going to have it on appHistory.transition anyway so having it earlier (before respondWith() is called) seems very reasonable.

Closes #86. Closes #11 by adding the appHistory.transition.finished promise.

Helps with #41 as you can retrieve data from the replaced entry, at least during the transition, with appHistory.transition.previous.

Probably helps with #14 (although currentchange needs a bit of an overhaul) since appHistory.transition has the sort of useful information discussed there, such as the previous entry or the type of navigation.
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@tbondwilkinson
Copy link
Contributor

Yay bike-shedding:

  1. I think all of these are pretty solid, though I still have a slight preference for "transition". I lean towards it because I like how the property access looks (appHistory.transition.type). If we want a combo, ongoingTransition or currentTransition is even more explicit.
  2. traversal is a little wordier, but is probably more accurate. No strong preference. If we go with "traversal" I think it should be "traverse" since the other types are verby (push, replace, traverse).
  3. Slight preference for "source" and "destination" terminology, though another option is "from" or "before". appHistory.transition.from is kinda nice.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
- `finished`: a promise which fulfills with undefined when the `navigatesuccess` event fires on `appHistory`, or rejects with the corresponding error when the `navigateerror` event fires on `appHistory`
- `rollback()`: a promise-returning method which allows easy rollback to the `previous` entry

Note that `appHistory.transition.rollback()` is not the same as `appHistory.back()`: for example, if the user goes back two steps, then `appHistory.rollback()` will actually go forward to steps. Similarly, it handles rolling back replace navigations by doing another replace back to the previous URL and app history state, and it rolls back push navigations by actually removing the entry that was previously pushed instead of leaving it there for the user to reach by pressing their forward button.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does rollback trigger a navigate event? I think it would be good to mention that here either way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it does! Will mention.

@domenic
Copy link
Collaborator Author

domenic commented Apr 2, 2021

"traverse" is definitely better than "traversal", so I've at least updated in that direction. I'm still willing to be swayed toward "go".

I like "from" a good bit better than "previous" so I'll switch to that for now. It has the appropriate connotations of being "in the past" while also clearly conveying the direction.

@domenic domenic merged commit 8342f7d into main Apr 5, 2021
@domenic domenic deleted the rollback branch April 5, 2021 21:42
@frehner
Copy link

frehner commented Apr 26, 2021

Pretty late to the party, but looking at implementing this and came across this question:

For replace navigations, e.g. appHistory.navigate({replace: true, state: {}}), what is AppHistory.transition.from? For example:

AppHistory.transtion.from === AppHistory.current // true?

etc.

[edit] oops sorry, forgot that we use getState() now so the state part is not applicable.

@frehner
Copy link

frehner commented Apr 28, 2021

Note that appHistory.transition.rollback() is not the same as appHistory.back(): for example, if the user navigates two steps back, then appHistory.rollback() will actually go forward two steps. Similarly, it handles rolling back replace navigations by reverting back to the previous URL and app history state. And it rolls back push navigations by actually removing the entry that was previously pushed, instead of leaving it there for the user to reach by pressing their forward button.

Also note that appHistory.transition.rollback() will itself trigger a navigate event. This means you can continue to centralize your response to navigations in the navigate event handler, i.e. rollback-initiated pushes/replaces/traversals go down the same application code path as other pushes/replaces/traversals.

If you call AppHistory.transition.rollback(), it will fire a new navigation and thus a new AppHistory.transition will be created for that navigation event, correct?

If so, what is the AppHistory.transition.type that's associated with that navigation? It doesn't seem like push | replace | transition really encapsulates what is going on, since a rollback will do things that push | replace | transition will never do.

Should perhaps an additional transition.type be added specifically for rollbacks?

@frehner
Copy link

frehner commented Apr 30, 2021

I like the idea of AppHistory.transition.finished being a promise, but as I've been working on it I think it can cause some undesired headaches as well. Specifically this part:

- If [the promise returned from navigate()] rejects, fire `navigateerror` on `appHistory` and reject `appHistory.transition.finished`.

If I'm understanding this correctly, it means that if someone wants to call AppHistory.navigate(), they are required to now have error handling for both the promise returned from navigate() and AppHistory.transition.finished, right? Otherwise they will have unhandled promise rejections?

Additionally, since AppHistory.transition is cleared after each transition, you'll have to redo this error handling every time you call navigate.

For example:

try {
  const navigationPromise = appHistory.navigate('/url')
  // since there are two promises we have to handle, and since .transition is not created until we call .navigate()
  // we have call .navigate() but not await it so we can then access .transition
  await Promise.all([navigationPromise, appHistory.transition.finished])
catch(err){
  // handle errors
}

Or am I misunderstanding this completely? I could be wrong here.

frehner added a commit to frehner/appHistory that referenced this pull request Apr 30, 2021
For some reason the unhandled promise reject function I setup in jest wasn't working, so it took forever to find.

I'm not entirely sure I like how you have to handle errors like this, and left a comment WICG/navigation-api#90 (comment)
@domenic
Copy link
Collaborator Author

domenic commented May 3, 2021

AppHistory.transtion.from === AppHistory.current // true?

Yeah, I think that's the idea!

If you call AppHistory.transition.rollback(), it will fire a new navigation and thus a new AppHistory.transition will be created for that navigation event, correct?

Correct.

If so, what is the AppHistory.transition.type that's associated with that navigation?

Rolling back a traverse (e.g. -2) is done with a traverse (e.g. +2). Rolling back a replace is done by replacing (with the previous URL/state/etc. values). And rolling back a push is done with a traverse (-1).

I like the idea of AppHistory.transition.finished being a promise, but as I've been working on it I think it can cause some undesired headaches as well. Specifically this part:

Good catch. We'd mark the promise as handled (equivalent to immediately doing appHistory.transition.finished.catch(() => {}) upon promise creation), as is done for the other cases like this on the web platform, notably in streams. So, you'd never get unhandled promise rejections from appHistory.transition.finished.

@frehner
Copy link

frehner commented May 3, 2021

And rolling back a push is done with a traverse (-1).

With the caveat that the rolled-back entry is removed, and you can no longer go forward as well - so, slightly different behavior than a normal (-1) traversal.

Not sure if that’s an important enough distinction for developers to know about or not though.

We'd mark the promise as handled (equivalent to immediately doing appHistory.transition.finished.catch(() => {}) upon promise creation)

Ah ok that sounds good. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants