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

Adding sugar for current index/can go back/can go forward #6

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

Adding sugar for current index/can go back/can go forward #6

domenic opened this issue Feb 3, 2021 · 7 comments · Fixed by #55
Labels
surface api Minor tweaks or additions to the API surface to make it nicer (without changing the model)

Comments

@domenic
Copy link
Collaborator

domenic commented Feb 3, 2021

(Moved from slightlyoff/history_api#25 by @SetTrend, summarized by me.)

It's currently slightly awkward to determine the index of the current entry, and whether you can go back and forward:

const index = appHistory.entries.indexOf(appHistory.currentEntry);
const canGoBack = index !== 0;
const canGoForward = index < appHistory.entries.length - 1;

It might be nice if this could be done with

appHistory.currentIndex; // just index? or currentEntryIndex?
appHistory.canGoBack;
appHistory.canGoForward;

or maybe we should add something to all the individual entries, like

appHistory.currentEntry.index;
appHistory.currentEntry.canGoBack; // isFirst?
appHistory.currentEntry.canGoForward; // isLast?

A use case where this is especially important is when you want to show your own navigation UI, such as when you're a widget inside an iframe (where the browser navigation UI is not very related to your UI). @SetTrend's specific example was a multi-step form embedded in an iframe.

@tbondwilkinson
Copy link
Contributor

I do think we should add index to Entry. But I don't think we need canGoBack and canGoForward.

const canGoBack = appHistory.currentEntry.index !== 0;
const canGoForward = appHistory.currentEntry.index < appHistory.entries.length -1;

Seems very reasonable to me, once we've added index to entries.

@SetTrend
Copy link

SetTrend commented Feb 12, 2021

From discussion @ slightlyoff/history_api#25:

it'll always contain the current entry, so it'll be of length 1.

I don't quite get the notion of a "current entry" in history.

  1. Why is there always at least one entry in the collection?
  2. What does the current entry represent?
  3. Shouldn't the current entry be mutable?

@domenic
Copy link
Collaborator Author

domenic commented Feb 12, 2021

Why is there always at least one entry in the collection?

Because you are always on a page. That page is the current entry. To have zero entries, you'd need to not be on a page, but then you wouldn't be able to run JavaScript.

What does the current entry represent?

The page you are on.

Shouldn't the current entry be mutable?

It's not clear exactly what you mean by this.

@domenic
Copy link
Collaborator Author

domenic commented Feb 12, 2021

But I don't think we need canGoBack and canGoForward.

Although it's pretty straightforward, I find the lack of symmetry in

if (appHistory.current.index !== 0) {
  await appHistory.back();
}

a bit displeasing.

if (appHistory.canGoBack) {
  await appHistory.back();
}

seems nicer to me...

@SetTrend
Copy link

SetTrend commented Feb 13, 2021

You know, I'm asking because ...

If the current entry represents the current page, then a design may choose to use the appHistory current entry state for its view model state and incrementally update the object members of the appHistory current entry state to reflect the current situation.

ViewState

So, instead of storing the state somewhere else and clone it in order to add it to the history using appHistory.update({ state }), it could just use appHistory.current.state directly for it's view model state.

However, a deep clone operation is required anyway when calling appHistory.update({ state }), because otherwise, the application would accidentally update a previous history entry's state members.

That's why I'm asking about the notion of the current entry.

  • Should there be an automatic deep clone operation involved with appHistory.update/push({ state });?
  • How can a JavaScript be prevented from altering a previous entry's state object's members?

@domenic
Copy link
Collaborator Author

domenic commented Feb 13, 2021

You may enjoy #17.

@domenic domenic added the surface api Minor tweaks or additions to the API surface to make it nicer (without changing the model) label Feb 17, 2021
@domenic
Copy link
Collaborator Author

domenic commented Feb 27, 2021

Minor question: if your AppHistoryEntry got disposed, should its index be -1 or null?

Similarly, if you're looking at the event.destination AppHistoryEntry in a navigate event, should its index be -1 or null or should it not exist as a property at all? (The latter would require using a different class than AppHistoryEntry but that's potentially a good idea for other reasons anyway.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
surface api Minor tweaks or additions to the API surface to make it nicer (without changing the model)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants