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

Bikeshedding "entries" #1

Closed
domenic opened this issue Feb 3, 2021 · 14 comments
Closed

Bikeshedding "entries" #1

domenic opened this issue Feb 3, 2021 · 14 comments
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#16 by @jakearchibald, but expanded by me.

An appHistory.entries property might be a bit confusing, since various "maplike" JS and web platform APIs have entries()/values()/keys() methods.

At a more general level, although the idea of "history entry" has a long history in specs and implementations, it is not currently exposed to web developers. So there's no API consistency constraint that forces us to choose that term. We could use something else like "item".

Another route would be to avoid naming these things as much as possible, as suggested by @Yay295 in WICG/proposals#20 (comment). E.g. instead of

appHistory.currentEntry;
appHistory.updateCurrentEntry(); // or appHistory.replaceCurrentEntry()? See #7
appHistory.pushNewEntry();
appHistory.oncurrententrychange;

event.upcomingEntry;
event.ongoingEntry;

event.destinationEntry;

we could have

appHistory.current;
appHistory.update(); // or replace(); see #7
appHistory.push(); // or add()
appHistory.oncurrentchange;

event.upcoming;
event.ongoing;

event.destination;

In that case we'd still have to figure out a name for what is currently appHistory.entries, but it becomes less important as it is surfaced in fewer places...

@tbondwilkinson
Copy link
Contributor

One possible suggestion: stack(), which returns a stack of entries. I think sticking with the word Entry is the right choice though, more broadly.

@domenic
Copy link
Collaborator Author

domenic commented Feb 11, 2021

I personally find the "stack" analogy confusing since you can inspect more than just the top of the stack, and the current entry might not even be the top of the stack. (If you've gone back a bit.) But people keep using it, so maybe it's just me....

@tbondwilkinson
Copy link
Contributor

Yeah, I agree that the word "stack" is not the traditional data structure terminology. But I do think that for years that's what documentation about history has called it - and this carries over even into native navigation.

domenic added a commit that referenced this issue Feb 12, 2021
* appHistory.currentEntry → appHistory.current
* appHistory.updateCurrentEntry() → appHistory.update()
* appHistory.pushNewEntry() → appHistory.push()
* currententrychange event → currentchange event
* event.upcomingEntry → event.upcoming
* event.ongoingEntry → event.ongoing
* event.destinationEntry → event.destination

This helps with #1, although appHistory.entries remains.
domenic added a commit that referenced this issue Feb 12, 2021
* appHistory.currentEntry → appHistory.current
* appHistory.updateCurrentEntry() → appHistory.update()
* appHistory.pushNewEntry() → appHistory.push()
* currententrychange event → currentchange event
* event.upcomingEntry → event.upcoming
* event.ongoingEntry → event.ongoing
* event.destinationEntry → event.destination

This helps with #1, although appHistory.entries remains.
@domenic
Copy link
Collaborator Author

domenic commented Feb 12, 2021

I merged #35 which shortens everything in the way suggested in the OP. The question remains what to do with appHistory.entries.

That is somewhat related to the discussions in #7, as once we introduce a "slot key", it might be better to make the whole thing maplike---i.e. have keys(), entries(), values().

@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 Apr 7, 2021

This isn't the most important issue out there, but I'd still kind of like to resolve it, especially after the rename in #92. Now we have an entries() function which, unlike some other web platform classes that follow the values()/entries()/keys() structure, returns an array.

Options:

  1. Stick with what we have, an array-returning entries().

  2. Stick with an array-returning function but change the name: I like items().

  3. Follow the web platform key/value iterable pattern and have keys()/values()/entries() methods:

    • keys() would return an iterable over the key value of each entry
    • values() would return an iterable over the actual AppHistoryEntry objects
    • entries() would return an iterable over [key, appHistoryEntry] pairs
    • NOTE: these methods would return iterables not arrays so appHistory.values()[0] would return undefined, which could be confusing. You'd have to do [...appHistory.values()][0] or equivalently Array.from(appHistory.values())[0].
    • This would also give you the ability to iterate over [key, appHistoryEntry] pairs using [Symbol.iterator](), so e.g. for (const [key, entry] of window.appHistory) { ... }
  4. Follow the web platform readonly maplike pattern, which is a superset of (3) but also adds get(), has(), size, and forEach().

My instinct is toward (2), but I'm open to other opinions.

@Yay295
Copy link

Yay295 commented Apr 7, 2021

I like 4 since it follows established patterns and provides a lot of functionality that would be harder to replicate with the other options.

@bathos
Copy link

bathos commented Apr 8, 2021

The fourth seems most natural/intuitive to me. I was under the impression that iterable<K,V> existed to support key-value collections whose keys aren’t unique (e.g. FormData, URLSearchParams) *. If keys are unique, I would always prefer maplike<K,V> personally.

(OTOH, I don’t think any of these options would specifically be “bad”.)


* These are also the ones that get the "FooIterator Prototype" objects that no two browsers seem to implement the same way currently. At least in theory, maplikes just use %Map.prototype.entries% I think, though maybe that’s a recent change or something cause neither Chrome or FF seems to do it.

@bathos
Copy link

bathos commented Apr 8, 2021

Regarding the “entry” term itself, I’m probably biased by familiarity, but it doesn’t strike me as being awkward or ambiguous in general. The trouble seems very specific to having an entries() method because it implies the JS/Web IDL entries contract so strongly. A name like AppHistoryEntry doesn’t seem affected by this.

To me, items() seems oddly generic for a name that’s not part of an existing collection contract (as far as I know anyway). It also looks a lot like item(), the conventional (I think? I feel like I’ve seen it in a bunch of places anyway) name for indexed property getters (e.g. https://dom.spec.whatwg.org/#dom-nodelist-item).

If the goal is to choose a totally generic name, that name would seem to already be values — but like you mentioned, it returns an iterator rather than an array. Is it specifically preferable for there to be an array-returning op? e.g. DX perception, perf concern?

Of course, you could define sequence<AppHistoryEntry> values() anyway and then if anybody complains, you can just say you were carefully following the precedent established by %Object.values% 😈

@domenic
Copy link
Collaborator Author

domenic commented Apr 8, 2021

Is it specifically preferable for there to be an array-returning op? e.g. DX perception, perf concern?

Random access (without the extra [...appHistory.entries()][i]) comes up a lot in the tests we are writing. But, I am not sure whether it would come up a lot in real code.

Some of the readme examples have things like appHistory.entries()[appHistory.current.index - 1] which also seem plausible. Similarly, being able to do things like appHistory.entries().find(e => somePredicateOn(e.getState())) or appHistory.entries().slice(0, appHistory.current.index) also seem plausible.

My main concern here is sacrificing mainline DX cases (random access, simple array method usage) in favor of some generic symmetry argument ("the platform has a lot of maplikes, maybe this should be one too"). So I'm not very interested in arguments like "it follows established patterns" or "seems most natural/intuitive". Instead I'd like to get a sense of the cost/benefit of the actual usage patterns. I.e.

  • Pro-array:
    • Easy random access
    • Array methods are easily available
  • Pro-maplike:
    • Easy lookup by key (appHistory.get(key) instead of appHistory.entries().find(e => e.key === key))
    • Easy length computation (appHistory.size instead of appHistory.entries().length)
    • Easy to get all the keys (appHistory.keys()), although I have no use case for this

To me this leans in favor of an array instead of a maplike, but I'm open to evidence that people would use the "pro-maplike" code patterns a lot and the "pro-array" coding patterns rarely.

@ljharb
Copy link

ljharb commented Apr 8, 2021

Iterator helpers seems like it’d address a lot of the DX gap from having an iterator over an array.

@tbondwilkinson
Copy link
Contributor

I lean array instead of maplike.

It feels to me that if you have a key, most of the time you already have the whole entry already as well, so the appHistory.get(key) entry feels moot.

I prefer entries to items, since I think an AppHistoryEntry is an entry, not an item. But I would be open to other names like "stack" or something.

@bathos
Copy link

bathos commented Apr 8, 2021

My main concern here is sacrificing mainline DX cases (random access, simple array method usage) in favor of some generic symmetry argument ("the platform has a lot of maplikes, maybe this should be one too"). So I'm not very interested in arguments like "it follows established patterns" or "seems most natural/intuitive". Instead I'd like to get a sense of the cost/benefit of the actual usage patterns.

This makes sense. When I said “seems most natural/intuitive to me” it wasn’t meant to constitute a supported argument for that solution. I was offering an “as a developer, I would probably be least surprised by...” data point because I’d (mis-)interpreted the enumerated options as a prompt for that kind of feedback.

The rest of that comment was more specific, and not related to how many maplikes do or don’t exist :) It pertains to the choice between 3 and 4 (i.e., if a key-value collection is used, whether it would be option 3 or option 4). As far as I can tell, iterable<K,V> (“pair iterator”) is “almost maplike, but keys aren’t guaranteed to be unique”. The keys are unique here if I understand right, so I’m not clear on why 3 would make sense over 4. This is moot if key-value isn’t the chosen API though.

@domenic
Copy link
Collaborator Author

domenic commented Nov 18, 2021

#9 (comment) is leaning me away from maplike.

@domenic
Copy link
Collaborator Author

domenic commented Jan 10, 2022

Let's stick with entries(), the class name AppHistoryEntry, and the concept name "history entry".

@domenic domenic closed this as completed Jan 10, 2022
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

No branches or pull requests

5 participants