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

Handling failed navigations #16

Closed
domenic opened this issue Feb 3, 2021 · 3 comments · Fixed by #37
Closed

Handling failed navigations #16

domenic opened this issue Feb 3, 2021 · 3 comments · Fixed by #37

Comments

@domenic
Copy link
Collaborator

domenic commented Feb 3, 2021

There are a few cases where navigations can fail:

  1. appHistory.back() when the current entry is the 0th entry
  2. appHistory.forward() when the current entry is the last entry
  3. appHistory.navigateTo(someKey) where someKey is not the key of any entry in the list (e.g., it's one that's been permanently evicted, as discussed in this section; or, it's just a random string like "not-a-key")

Currently the proposal indicates that (1)-(3) will have the promise immediately fulfill with undefined, in a way that's indistinguishable from a success. The proposal explains you can check after the fact as to whether the navigation actually worked:

const startingEntry = appHistory.currentEntry;
await appHistory.back();
if (startingEntry === appHistory.currentEntry) {
  console.log("We weren't able to go back, because there was nothing previous in the app history list");
}

It's unclear whether the current proposal gives a good design. I think at least (3) should result in a rejected promise.

I'm less sure about (1) or (2). Possibilities include:

  • Current proposal (always fulfill promise with undefined)
  • Fulfill the promise with a boolean indicating whether or not navigation succeeded
  • Reject the promise if navigation failed. (This would be more palatable if we introduce the canGoBack and canGoForward sugar from Adding sugar for current index/can go back/can go forward #6.)

I don't have a strong opinion on which of these is best.

@SetTrend
Copy link

SetTrend commented Feb 4, 2021

In continuation of our discussion on Navigation through the app history list, let me suggest an enum to provde programmers a with a failure reason:

const enum NavigateFailureReason
{
  NoError    // entry was set successfully
,  Error   // navigate() threw an exception
,  Cancelled    // navigate() was cancelled
,  InvalidHistoryEntry    // non-existend key
,  OutOfHistory    // .back()/forward() beyond entry array
}

domenic added a commit that referenced this issue Feb 5, 2021
Closes #23. Closes #24.

Note that the failed-back detection might change per #16.
@tbondwilkinson
Copy link
Contributor

I like 3) here - I think the Promise should reject in any case where the navigation did NOT occur as intended. So if your back is not possible, it didn't happen, and the Promise should reject.

I do think that the rejection reason should be helpful and tell the caller WHY the navigation didn't occur (cancelled by the router, not possible given the stack, etc.)

domenic added a commit that referenced this issue Feb 11, 2021
Closes #23. Closes #24.

Note that the failed-back detection might change per #16.
domenic added a commit that referenced this issue Feb 12, 2021
Closes #23. Closes #24.

Note that the failed-back detection might change per #16.

This greatly expands on the integration with joint session history, in particular noting how traversal still uses the joint session history and thus can impact other frames.
@domenic
Copy link
Collaborator Author

domenic commented Feb 12, 2021

I'm working on this now. I don't think we'll distinguish between an invalid key passed to navigateTo(), and being unable to go back()/forward(), because they're basically the same: you're trying to go to an entry that doesn't exist. We can revisit that if there are strong use cases (e.g. example code based on a real application).

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 a pull request may close this issue.

3 participants