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

New API with getOrInsert and getOrInsertComputed #58

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

dminor
Copy link
Collaborator

@dminor dminor commented Oct 17, 2024

This updates the design to match what was presented to committee at the October 2024 plenary. I am becoming champion of the proposal with the students at the University of Bergen as co-authors, and the previous champion and author are noted as no longer actively involved in the proposal.

The naming of the methods is still up for discussion, I plan to open an issue to discuss options once this pull request is merged.

This updates the design to match what was presented to committee
at the October 2024 plenary. I am becoming champion of the proposal
with the students at the University of Bergen as co-authors, and the
previous champion and author are noted as no longer actively
involved in the proposal.

The naming of the methods is still up for discussion, I plan to
open an issue to discuss options once this pull request is merged.
README.md Outdated
@@ -1,129 +1,126 @@
# `Map.prototype.emplace`
# `Map.prototype.getOrInsert`
Copy link
Member

Choose a reason for hiding this comment

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

Please choose a proposal name that doesn't tie us to a specific solution. This is an example of the issue, as the proposal problem space hasn't changed, but now we're having to rename the proposal. I am usually pretty strict about this during Stage 1 presentations, but this proposal might predate me being so diligent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Completely agree, would Map Default Values be ok?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that sounds good to me. That would also permit solutions we explored (and didn't like) like initialising a Map with a default value.

Choose a reason for hiding this comment

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

I think matching the repository name "upsert" is fine: https://en.wiktionary.org/wiki/upsert

To insert [rows into a database table, or more generally items into a collection] if they do not already exist, or update them if they do.

Copy link
Member

Choose a reason for hiding this comment

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

I think "upsert" is a great proposal name regardless of the eventual solution.

Copy link
Member

Choose a reason for hiding this comment

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

It does? Points 1 and 4 combined are "get or insert" and 2 and 4 combined are "get or insert computed".

I definitely agree on naming broadly, I just think upsert is sufficiently broad.

Copy link
Member

Choose a reason for hiding this comment

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

In my above comment, I was referring to an API like new Map(..., { default: () => 0 }), called DefaultMap in Dan's slides, referencing Python's defaultdict.

Copy link
Member

Choose a reason for hiding this comment

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

ah, fair point.

Copy link

@gibson042 gibson042 Oct 17, 2024

Choose a reason for hiding this comment

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

I think "upsert" covers that approach at least as well as "Map Default Values" covers map.getOrInsert(key, initialValue) (i.e., not perfectly but good enough to make sense).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm ok with either option, neither is perfect, the one advantage of upsert is that it matches the repository name which might help people find things.

README.md Outdated
@@ -1,129 +1,126 @@
# `Map.prototype.emplace`
# `Proposal Upsert`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# `Proposal Upsert`
# Proposal Upsert

@dminor dminor requested a review from michaelficarra October 21, 2024 23:01
Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

WFM. I'm not gonna do a review of the spec text until after Stage 2. I would like to see an API name bikeshedding issue at some point though. There's lots of prior art for us to take naming inspiration from and hopefully make it intuitive/unsurprising for as wide a group as possible.

@dminor
Copy link
Collaborator Author

dminor commented Oct 22, 2024

WFM. I'm not gonna do a review of the spec text until after Stage 2. I would like to see an API name bikeshedding issue at some point though. There's lots of prior art for us to take naming inspiration from and hopefully make it intuitive/unsurprising for as wide a group as possible.

Thanks. I was planning to open an issue to bikeshed the names as soon as this is merged.

@dminor dminor merged commit dc0a42c into tc39:master Oct 22, 2024
@dminor dminor deleted the get-or-insert branch October 22, 2024 14:50
zloirock added a commit to zloirock/core-js that referenced this pull request Oct 27, 2024
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 this pull request may close these issues.

5 participants