Thoughts And Concerns Of Effects of Proposal

[Skillz4Killz]

Hello 👋

Although I am a fairly young dev with a long way to go, I was hoping to try and be able to give my point of view regarding this proposal.

I have a few different concerns regarding this proposal.

Simplicity

I believe that this change could ruin the simplicity and beauty of Maps in JS. All the current methods in JS Maps are all very simple methods. They either accept no arguments, a key, a key/value. The only method that takes a callback is .forEach and that is only because you are trying to function on the entire map itself which in my opinion wasn't needed. You can put a for loop on map.values().

Upsert takes in 3 arguments already more than anything else, which isn't that big of an issue but two of them are callbacks. Why? This proposal doesn't make the current solution simpler but more complex. The fact is the proposal in its current form is a lot scarier(to a beginner dev like me) and doesn't seem as pleasant on the eyes to make me want to ever use it. Although it does reduce the lines of code down to one line I can achieve the same result by creating my own function in my project to replicate his behavior if so desired.

If the value exists, then it returns the value otherwise it adds the value using a map.set() and then returns the value which would result in.

map.upsert(key, value).doThing()

Maps are one of if not the most beautiful part of JS! Simplicity is beautiful.

Cluttering

In my opinion, this method does not add much to the API except clutter. The same functionality is already achievable. Upsert is just a simpler way of achieving it. The reason I want to bring this up is that the last couple of days, I have been listening to lectures from Douglas Crockford where JS went wrong by creating so much clutter.

Any developer desiring this functionality can easily create a function or a method on their own extended version of a Map in their own project to achieve the same result. Make that extended Map an NPM module and now you can use it on any of your projects. The overall point I am hoping to make is that the functionality is already achievable and this makes JS unnecessarily cluttered as opposed to keeping it simple.

Unclear Motivation

The workarounds involve multiple lookups and developer inconvenience. In my opinion, this proposal is the workaround that is creating developer inconvenience.

The current form is clean and simple and can be understood at first glance immediately. When you look at your code 6 months from when you code it, it should be easily understood what it is doing. You can look back in 6 years and still be able to understand it because it flows nicely.

Simplicity is everything. JS is a beautiful language, let's keep it that way.

Performance

I think the main reason behind most of the reasons shown in the readme is having extra lookups. In my opinion, Maps are already super fast. Having to do 1-2 lookups is not a bad thing at all when you can already perform like a hundred million operations per second.

Here are some JSPerfs I tested to show: (Test it yourself as well, this was the first time I ever needed to do JS testing like this. Perhaps I made some mistakes when doing it. What I found was that maps run millions of times a second.)

Always set a new value:

Reading Values: (90% of the time you will be doing this)

P.S. I hope this doesn't seem to come off too negatively. I think it's a great goal to make the dev life easier but I am unsure if this is the right way to do it.

Thank you :)

[resynth1943]

Thanks for writing this. I've seen you apply your programming skills (pardon the pun) on another social media platform, so I wouldn't dismiss your opinion :-)

Anyway, I really don't think this proposal is a good idea, and it's definitely a horrible workaround, if you may. The current name, upsert, does not provide any information to the reader on the function the method actually performs. In contrast with the existing API, this wouldn't complement the readable API. Regardless of whether or not upsert is a "standard" term, it still doesn't feel readable enough for implementation.

Following this, the functionality this method provides is barely worth implementing a method, in my opinion. The README uses this as an example:

if (!map.has(key)) {
  map.set(key, value);
}
map.get(key).doThing();

but what would be wrong with doing this?

if (!map.has(key)) {
  map.set(key, value);
}

value.doThing();

Furthermore, I completely agree with your points on functionality. This method would provide little to no value, and would require yet more polyfills.

I'll leave you with this thought: Why not do it the old-fashioned way?

Many thanks, Will

[ljharb]

"upsert" is a commonly-used idiom for "update or insert if exists" in SQL.

Map's "simplicity" is not a good thing, it's part of ES6's "max/min" philosophy, where getting the low-level primitive in the language was the priority. There is a painful dearth of convenience methods on Map and Set, and this proposal - along with some others - attempts to address it.

[Skillz4Killz]

There is also a massive lack of convenience methods on arrays, objects, and strings. Entire modules have been made to provide these convenience methods. For example, Lodash, currently 23 million downloads weekly.

This proves that there is a serious lack of convenience methods that Lodash provides to millions of developers. I don't believe this means that we should take all the Lodash(and other similar libs) code and clutter the JS codebase to provide those convenience methods in the JS core.

Respectfully, in my opinion, convenience methods should not clutter the JS language. They should be made available through an NPM module that anyone can use on any project. The core JS language should be kept pure and simple.

[thumbsupep]

I thought the only reason Map had fewer methods than arrays and objects was because Map is much newer to the language? That was just an assumption though. Here is the value upsert would add: Map has neither update nor insert-if-missing functions. upsert would accomplish both and would allow developers to use native js to do so instead of importing a massive library like lodash. Array can apply updates with .map, Strings can .replace, and many other languages have functions similar to upsert. There is currently no catch-all way to update or insert-if-missing in a Map.

I'm new to TC39, but I was under the impression that the committee helps expand the stdlib and that's why we discuss these proposals. I find the perspective of dont-add-this-method-because-we-shouldnt-add-methods to be unconvincing.

I acknowledge that has + get + set is a valid way to accomplish upsert. I would say the value add for upsert w.r.t to that argument would be developer convenience (one line instead of 3 or more; no importing extra modules). And, as @Skillz4Killz noted, minimal optimization since maps run millions of times a second.

If you're opposed to convenience methods, upsert is pretty benign because it only adds one when it could exist as two. So the saving grace is it's a 2 for 1!

[thumbsupep]

I am also totally open to name changes for upsert.

[favna]

If anything upsert just makes me upset.

yes pun intended

[Skillz4Killz]

Anyone can make their own small library for extending map functionality. Adding this in either a module or the core JS makes no difference. One way or another the upsert is going to cause clutter. It would be better to have that in a module that developers "choose" to add to their code vs adding it to the core language that developers lose the ability to "choose" and are stuck with a massive cluttered language. I believe having it in a library gives much more freedom for devs.

I find the perspective of dont-add-this-method-because-we-shouldnt-add-methods to be unconvincing.

I agree. I apologize if I was not expressing myself well. I was trying to respond to ljharb response which I understood to say that basically, the only reason to add this was developer convenience. What I was trying to say was that if the only reason to add something is developer convenience, this proposal should not happen.

Lodash is an example that has proven that millions of developers are okay using a lib even as massive as Lodash to provide convenient methods. I see no valid reason for the core JS language to be cluttered.

one line instead of 3 or more; no importing extra modules

The question I was hoping to raise is: Is this reason enough to add something to the core language? Lodash does this for so many different methods. Should we move the entirety of Lodash and other similar libs into the core JS language? I personally would be against that as well. I don't think to add to the core language for convenience alone. The performance factor, as shown above, is already basically irrelevant. Maps are too fast to be hindered by an extra lookup or two.

• There is no new functionality added.
• I, possibly others, could argue that the current method is much easier and more maintainable.
• The performance impact is negligible.

Why are we adding this to the core language?

[ljharb]

(I think you'll find the majority of lodash is already in the language)

[hax]

@ljharb If majority of lodash is already in the language (actually I agree with this point) , why people always need lodash ? 😂

[hax]

@Skillz4Killz What do you think of alternative design like:

const store = new Map([['apple', 1]])

const apple = store.entryView('apple')
apple.exist() // true
apple.key // 'apple'
apple.value // 1

if (apple.exist()) ++apple.value
else apple.value = 0

apple.value // 2
store.get('apple') // 2

const banana = store.entryView('banana')
banana.exist() // false
banana.key // 'banana'
banana.value // undefined

if (banana.exist()) ++banana.value
else banana.value = 0

banana.value // 0
store.get('banana') // 0

Basically, for x = map.entryView(key), x.key is just key, x.value is just map.get(key) x.value = foo is just map.set(key, foo) x.exist() is just map.has(key)

I feel this API is still simple enough and "can be understood at first glance immediately". But it avoid multiple lookup explicitly (which good for engine optimization I hope), and provide some convenience (I hope) if programmers want to operate on special entry continuously.

[ljharb]

Exposing a mutable object whose mutations affect the Map does not seem simple or desirable to me (if I’m understanding your suggestion correctly).

[hax]

Exposing a mutable object whose mutations affect the Map

@ljharb

Yes this is what xxxView mean. Just like TypedArray/DataView.

does not seem simple

"Simple" has many meanings. I understand @Skillz4Killz 's "simplicity" mainly mean only exposing fundamental operations of an interface. I see a map is a group of entries so exposing an entryView seems still fall in the category of fundamental operations and do not add any new concept.

On the other hand, upsert() is a compound operation which provide a new, high-level abstraction which require two optional callback parameters, much complex than all current methods on Map.

upsert() could be easily implemented with the entryView, but not vice versa. It's also possible to use entryView to do something like if (entryView.value === 0) entryView.delete() which upsert() don't cover. Almost all apis in other languages could be built based on entryView.

Map's "simplicity" is not a good thing, it's part of ES6's "max/min" philosophy

Personally I still think "max/min" philosophy is the good way. Even we want to add some high-level functionality to meet some use cases of upsert, I feel we could first consider more fundamental api like entryView which also cover all use cases of it. Then allow libraries use these fundamentals to create new APIs in different tastes and let developers try, see what is most suitable for JS ecosystem and then decide whether we need to add them to Map.

[bmeck]

I'm not comfortable exposing a direct Entry API and have stated so in the meetings when this proposal has been brought up. If you want such an API please make a separate proposal as that needs to account for a variety of extra scenarios such as if we introduce read only collections, collection normalization hooks, entries and affects of GC on collections, forcing collection underlying implementation constraints such as pinning keys permanently, etc. If you find this proposal to be problematic in creating such an Entry API, please let us know and we have mentioned the need to investigate forwards compatibility with any such API being introduced.

Reading these comments I think we have already answered them in the FAQ, but please let me know if we need to add more FAQ entries or if you disagree with the reasoning in the FAQ or feel that the uses mentioned in the FAQ are invalid in some way.

If the value exists, then it returns the value otherwise it adds the value using a map.set() and then returns the value which would result in.

map.upsert(key, value).doThing()

Wouldn't handle things that are primitives or value types:

// how do i add 1 to the keyed entry?
const fnCalls = new Map(); // Map<Function, Number>
fnCalls.upsert(fn, 1); // seems fine for inserting if missing
fnCalls.upsert(fn, 1); // cannot be used add + 1 to the value and store back in fnCalls

This is an important use case as value types are around the corner in a variety of forms and even recent types like BigInt fall into that category.

Any developer desiring this functionality can easily create a function or a method on their own extended version of a Map in their own project to achieve the same result. Make that extended Map an NPM module and now you can use it on any of your projects. The overall point I am hoping to make is that the functionality is already achievable and this makes JS unnecessarily cluttered as opposed to keeping it simple.

That is true of most APIs as JS is turing complete. I'm not exactly sure how making a bit of precarious boilerplate that isn't uniform as shown in your JSPerf examples (3 different ways of doing one basic workflow) helps users conform or learn a standard way of doing things. Having APIs that serve a common usage problem is important and this proposal seeks to solve that for updating or inserting into Map collections. If a user wishes to create a new function or method on their own, they can continue to do so and this proposal should not cause any problems.

I think the main reason behind most of the reasons shown in the readme is having extra lookups. In my opinion, Maps are already super fast. Having to do 1-2 lookups is not a bad thing at all when you can already perform like a hundred million operations per second.

That is only 1 reason, the developer workflow convergence and ergonomics continue to be other points of interest. I'm not sure what this comment is trying to state except that performance might be less important than other gains that this proposal allows.

[Skillz4Killz]

@hax I believe your approach is much simpler and cleaner, but I still feel as if it is too much and possibly unnecessary. I still do not see what is wrong with the current approach. Both solutions seem to me like a solution to a problem that doesn't actually exist.

Personally I still think "max/min" philosophy is the good way.

Same. I also love the minimal simplicity.

I think you'll find the majority of lodash is already in the language

@ljharb I think I am failing to express myself again. Sorry.

Implementing something from lodash or another lib into the core is not a bad thing IF that brings about real impact to the QOL for devs. Adding things to the core JS language for no real impact/value is just adding clutter. I just don't feel this proposal has that necessary impact to belong in the core JS language and this is why I believe it should be preferred as an NPM Module.

please let me know if we need to add more FAQ entries or if you disagree with the reasoning in the FAQ or feel that the uses mentioned in the FAQ are invalid in some way.

@bmeck Thank you for taking the time for that. I do agree with quite a bit of what you said but there are parts where I am still confused. I am going to try and explain how I understood the proposal while reading the README to better explain myself.

if (!map.has(key)) {
  map.set(key, value);
}
map.get(key).doThing();

I wouldn't write that code spread across 4 lines but instead 2 lines. Also, I wouldn't make an extra get() at the end as I already have the value since we are trying to set that value. Even if I have misunderstood that part and you do need the .get() then it still keeps it 2 lines and as I showed earlier maps are too fast to matter with 1 extra get. To be safe I will write the get().

// The way I would write this code
if (!map.has(key)) map.set(key, value);
map.get(key).doThing();

You see, I see nothing wrong with this code in any shape or form. I could read this code 6 months from now and immediately understand what it's trying to do. It is very clear, simple, and to the point.

The proposed solution changes this dramatically in my opinion.

map.upsert(key, o => o, () => value).doThing();
// or
map.upsert(key, undefined, () => value).doThing();

The first thing that immediately jumps at me is the undefined. This is very troublesome for me(probably for others as well if I had to guess). If I imagine myself reading that code 6 months from now this is what I believe my process would be like:

1. Get's confused about what undefined is. What is that parameter that I marked undefined?
2. Open my browser go to mdn docs
3. Search for Map.upsert.
4. Read the upsert docs to figure out what the second parameter is meant to be.
5. Go back to VSC
6. Read the code again knowing that the second parameter is the callback to handle updating if the value exists.
7. Figure out the changes I need to make. Make changes.

This is already throwing up red flags to me. I already see this proposed solution as breaking the simplicity and beauty of the current Maps.

Also, this is not just about undefined. This same process holds true for the first one. The code is not explicit in explaining itself. What is o? I need to also figure that out. Is the second the update or the third arg, the update handler? Is the second the insert handler or the third arg the insert handler? It's not clear from reading the code. This is something devs need to memorize or lose time having to go read docs.

You can leave a comment explaining it which is a decent solution. But that brings back the same amount of lines as the current way I would write this code. 2 lines.

Next point:

map.upsert(key, o => o, () => value).doThing();

I don't feel like the handlers in this example are accurately showing how this would actually work. The two callback handlers here make no sense to me. I am assuming they are just placeholders for larger callbacks. Please correct me if I am wrong on this.

An example, I would be slightly more open to (but still believe the current solution is the best)

map.upsert(key, { insert: () => value })

This would immediately make it understandable code to me. I would know that () => represents the insert handler. I would also remove the confusion caused by having the undefined in the args. Although this still requires a bit of memorization and looking into docs in order to modify it, I believe this is much more readable than the current proposal. Just to be clear, I still prefer the simpler, cleaner current version.

You can apply the same logic and reasoning to all the other examples shown in the README. This is how I understand the proposal. If my understanding of it is wrong, then please let me know.

That is only 1 reason, the developer workflow convergence and ergonomics continue to be other points of interest.

This is the main part I am not convinced about. I don't feel this proposal is actually improving these things. I actually believe it is making it worse.

I do not believe I would ever use upsert as it makes code much harder to read and therefore maintain. I love the current way of doing it. There is absolutely nothing wrong with it IMO. From my point of view, this proposal does not really increase developer QOL or efficiency.

---

how do i add 1 to the keyed entry

Side Note: To answer your question, honestly, I feel like this complexity should be separated to keep the simplicity of Maps. If I needed an increment solution then id build one: map.increment() or map.addOne(). Simplicity is everything.

You all are way more experienced than me so I trust your judgment on this. I hope I have successfully expressed my(possibly other beginner devs) point of view. Sorry, if I keep writing novels for you to read. I just really LOVE Maps and feel very passionately about them. :)

[bmeck]

// The way I would write this code
if (!map.has(key)) map.set(key, value);
map.get(key).doThing();

Yes, the .get is needed for simplicity; consider the following:

const people = new Map([]);
function addPerson(name) {
  if (!people.has(name)) map.set(name, new Person());
  people.get(name).doPeopleThing();
}

If we don't use the .get your code gets more complicated:

const people = new Map([]);
function addPerson(name) {
  if (!people.has(name)) {
    let person = new Person();
    map.set(name, person); // we don't want the return value of doPeopleThing
    person.doPeopleThing();
  } else {
    people.get(name).doPeopleThing();
  }
}

---

The first thing that immediately jumps at me is the undefined. This is very troublesome for me(probably for others as well if I had to guess). If I imagine myself reading that code 6 months from now this is what I believe my process would be like:

1. Get's confused about what undefined is. What is that parameter that I marked undefined?
2. Open my browser go to mdn docs
3. Search for Map.upsert.
4. Read the upsert docs to figure out what the second parameter is meant to be.
5. Go back to VSC
6. Read the code again knowing that the second parameter is the callback to handle updating if the value exists.
7. Figure out the changes I need to make. Make changes.
8. This is already throwing up red flags to me. I already see this proposed solution as breaking the simplicity and beauty of the current Maps.

Also, this is not just about undefined. This same process holds true for the first one. The code is not explicit in explaining itself. What is o? I need to also figure that out. Is the second the update or the third arg, the update handler? Is the second the insert handler or the third arg the insert handler? It's not clear from reading the code. This is something devs need to memorize or lose time having to go read docs.

This seems like we could improve naming or ban default callbacks and require a value! Both perhaps? However, I would note these arguments are true for almost every API; you must learn the API to use it (e.g. learning what Array#filter does with its parameters and the result of the callback). Improvements and suggestions are welcome if naming could be improved or if you feel we should ban defaulting the arguments.

I don't feel like the handlers in this example are accurately showing how this would actually work. The two callback handlers here make no sense to me. I am assuming they are just placeholders for larger callbacks. Please correct me if I am wrong on this.

An example, I would be slightly more open to (but still believe the current solution is the best)

map.upsert(key, { insert: () => value })

This would immediately make it understandable code to me. I would know that () => represents the insert handler. I would also remove the confusion caused by having the undefined in the args. Although this still requires a bit of memorization and looking into docs in order to modify it, I believe this is much more readable than the current proposal. Just to be clear, I still prefer the simpler, cleaner current version.

I don't think any other APIs are naming their parameters in this way. See things like Array#filter(selectFn) and Promise#then(fulfilledFn, rejectedFn) not using objects with properties. This falls into learning APIs again I believe. If you can suggest how to make this API more learnable that would be great! However, I don't feel that using objects for the API is currently desirable when you compare with other APIs in the language.

---

This is the main part I am not convinced about. I don't feel this proposal is actually improving these things. I actually believe it is making it worse.

I'd disagree. We work a lot with in-memory caches using Maps at my work and largely feel pain from needing to carefully ensure we use

• the same boilerplate conditions (always check .has don't check for undefined)
• ensure we properly and lazily allocate expensive or shared values

This API would greatly improve our ability ensure this workflow doesn't diverge across our codebase and would allow common gotchas like eagerly allocating values to be avoided.

I do not believe I would ever use upsert as it makes code much harder to read and therefore maintain. I love the current way of doing it. There is absolutely nothing wrong with it IMO. From my point of view, this proposal does not really increase developer QOL or efficiency.

You don't have to use it! We are not planning to break any existing APIs :).

---

Side Note: To answer your question, honestly, I feel like this complexity should be separated to keep the simplicity of Maps. If I needed an increment solution then id build one: map.increment() or map.addOne(). Simplicity is everything.

But then if you need to add either +1 or +2 or "/" you start getting many methods. Once you have many methods you start needing to learn which are available and what the different names are for each. E.g. Is it increment or addOne or next or ... ? It isn't simple once you do the same workflow in multiple ways.

Even if these are added you do end up with that boilerplate about .has still as well:

function add(name) {
  if (!map.has(name)) map.set(name, 0);
  map.addOne(name);
}

The point here is to abstract that generic workflow so that things like addOne can follow consistent rules about when to insert default values and how to do updates. It would be bad if the addTwo alternative API of the above add function didn't follow those same conventions and you ended up with something like:

function add2(name) {
  const existing = map.get(name);
  if (!existing) map.set(name, 0);
  map.addTwo(name);
}

If your map had an existing "falsey" value like NaN add2 would reset the value to 0, which would be inconsistent with add.

[resynth1943]

In your example, you use the following code to illustrate your point:

const people = new Map([]);
function addPerson(name) {
 if (!people.has(name)) {
   let person = new Person();
   map.set(name, person); // we don't want the return value of doPeopleThing
   person.doPeopleThing();
 } else {
   people.get(name).doPeopleThing();
 }
}

What would be the issue with using the below code?

const people = new Map([]);
function addPerson(name) {
  const person = new Person();
  if (!people.has(name)) map.set(name, person); 
  person.doPeopleThing();
}

Could you elaborate on this?

[bmeck]

@resynth1943

In your example

• it always calls new Person which means you allocate a completely different person every time that addPerson is called. This could be bad if allocating a person is an expensive operation or has side effects (perhaps does DB work to allocate something or is just a complex class).
• it doesn't actually update the map if the person with that name exists. It never really does an update to any existing entry's Person in the map.
• it always calls person.doPeopleThing() on the new person, never on an existing person in the map. so if you intended for a person with that name already in the map to update it wouldn't. Additionally, if you have side effects in person.doPeopleThing() it could cause problems.

Your form is more like an "insert if missing but always create a person and do something with the new person"; it never really does any update operation.

[resynth1943]

@bmeck Ahh, thanks for the clarification, had a brain-freeze moment.

[Skillz4Killz]

Would it not be simpler to do this:

const people = new Map([]);
function addPerson(name) {
  const person = people.get(name);
  if (person) person.doPeopleThing();
  else {
    const newPerson = new Person();
    map.set(name, newPerson);
    newPerson.doPeopleThing();
  }
}

I actually still believe that this and the one you wrote are better than the proposal upsert method. This code is explicit and has no need for comments/docs or any sort of explanation of what it's doing.

However, I would note these arguments are true for almost every API; you must learn the API to use it (e.g. learning what Array#filter does with its parameters and the result of the callback).

I agree that there is always a barrier to learning but I don't believe that this is a good comparison. Array#filter and Map#upsert have very different levels of comprehension required. Array#filter is a self-explanatory code, whereas Map#upsert is not.

peopleArray.filter(person => person.isMale)
peopleMap.upsert('skillz', o => o, () => value)

Even if I had no idea what Array.filter does, I would understand it just by reading that line of code. On the other hand, upsert is the opposite.

This falls into learning APIs again I believe. If you can suggest how to make this API more learnable that would be great!

I believe Maps are already a great tool that you can accomplish everything you need in a very clean and simple way. Sometimes, you can reach a point when refactoring code too much can be a harmful thing. When code stops being easily understood, you have refactored too much.

Most of my time is spent reading code. Once I begin to write code, I actually write quite fast. Explicit code is the best form of code. I don't want to spend minutes trying to search docs about what this line is doing.

We work a lot with in-memory caches using Maps at my work and largely feel pain from needing to carefully ensure we use... This API would greatly improve our ability ensure this workflow doesn't diverge across our codebase and would allow common gotchas like eagerly allocating values to be avoided.

Why not make a package for your work and add the upsert functionality as a package on NPM?

As an example, I have a similar pain of having to deal with map#find similar to array#find. I use maps a ton and I just simply extended the Map class and added a find function. That solved all my issues.

You don't have to use it! We are not planning to break any existing APIs :).

Sadly, this still is a problem. Just because I don't use it, does not mean that I do not need to learn it or understand it. Someone could make an NPM module with this and when I'm trying to read that module's code to understand it and see if I want to use it, I need to read it initiating the 8 step process to understanding what this line is doing. Or maybe I get hired to work by a company with an existing codebase using this.

---

But then if you need to add either +1 or +2 or "/"

In this case, I would simply not have created a .increment or .addOne. Maps come with a beautiful built-in method to handle all of this. Map#set solves all of this. If I needed something to always increment by 1 is when I would create a custom method for that. If it needs to be dynamic, that's already built-in functionality.

[bmeck]

const people = new Map([]);
function addPerson(name) {
  const person = people.get(name);
  if (person) person.doPeopleThing();
  else {
    const newPerson = new Person();
    map.set(name, newPerson);
    newPerson.doPeopleThing();
  }
}

This suffers from the same NaN and missing entry issues as discussed above. It is not a suitable replacement as undefined/falsey values differ from a key not existing in a Map.

Array#filter is a self-explanatory code, whereas Map#upsert is not.

We actually had a presentation at TC39 this month about how Array#filter isn't self-explanatory : see https://github.com/jridgewell/proposal-array-select-reject

I believe Maps are already a great tool that you can accomplish everything you need in a very clean and simple way. Sometimes, you can reach a point when refactoring code too much can be a harmful thing. When code stops being easily understood, you have refactored too much.

We are open towards any recommendations on allowing this API to be easier to understand :).

Most of my time is spent reading code. Once I begin to write code, I actually write quite fast. Explicit code is the best form of code. I don't want to spend minutes trying to search docs about what this line is doing.

I don't think this is a comment about this proposal in general, but on not wanting to have many APIs in the language itself.

Sadly, this still is a problem. Just because I don't use it, does not mean that I do not need to learn it or understand it. Someone could make an NPM module with this and when I'm trying to read that module's code to understand it and see if I want to use it, I need to read it initiating the 8 step process to understanding what this line is doing. Or maybe I get hired to work by a company with an existing codebase using this.

This is true of all APIs not just limited to the JS language but also the ecosystem. I often have to look up what APIs from 3rd party modules do. This doesn't seem actionable feedback nor do we want to prevent the ecosystem or language from adding new APIs.

If you have suggestions on how we can make the workflow more intuitive while maintaining the fidelity of the API we are open to changes such as banning defaulting values; however, it doesn't seem to be actionable to state that we need to stop relying on programmers to learn or be able to look up APIs as that would halt all progress of introducing APIs.

In this case, I would simply not have created a .increment or .addOne. Maps come with a beautiful built-in method to handle all of this. Map#set solves all of this. If I needed something to always increment by 1 is when I would create a custom method for that. If it needs to be dynamic, that's already built-in functionality.

This goes against the point made above which my comment was made in response to. I don't see anything that is related to this proposal if there are arguments being made both for and against creating specialized methods. If there is an example of by what criteria you would/would not make APIs and how that relates to this specific proposal that seems possible to ensure that this proposal does have the ability to respond to in its FAQ.

[ljharb]

@Skillz4Killz it's not simpler - you had to repeat people.doPeopleThing() twice. Repetitions cause bugs because they're easy to mess up.

[Skillz4Killz]

This suffers from the same NaN and missing entry issues as discussed above. It is not a suitable replacement as undefined/falsey values differ from a key not existing in a Map.

I would be very shocked if someone had a falsy value for a map holding Person values. In that unlikely case, there is room to have the code be optimized in some manner. I don't allow my maps to be set with any falsy value. Therefore, that issue would never arise and I believe .get() is safe to use here if you handle your maps nicely.

We are open towards any recommendations on allowing this API to be easier to understand

Honestly, I think your design of the proposal is great. It's not that the way it's being done is wrong but that upsert is trying to achieve in one line so much its impossible to keep it simple.

This goes against the point made above which my comment was made in response to. I don't see anything that is related to this proposal if there are arguments being made both for and against creating specialized methods. If there is an example of by what criteria you would/would not make APIs and how that relates to this specific proposal that seems possible to ensure that this proposal does have the ability to respond to in its FAQ.

I don't think any of my custom methods should be created in the core of JS. In fact, I think my solution to having my needs from Maps in an NPM module is perfect and I can use that anywhere. I believe that Maps are already great and don't need an increment or addOne or upsert to overcomplicate and remove the simplicity. I believe this is custom functionality that doesn't belong in the core.

We actually had a presentation at TC39 this month about how Array#filter isn't self-explanatory : see https://github.com/jridgewell/proposal-array-select-reject

Hmm, had a look at this and I think maybe I just have a very different mentality. I'm quite confused about why the shift has been made from the ES6 style. I believe that was the best style that could make JS perfect by keeping it simple. Perhaps I am the only one who feels about this like this and if so then I would say this proposal is good to go.

I think this is also saying what I am saying but in different phrasing. https://github.com/jridgewell/proposal-array-select-reject/issues/5

I think perhaps this is not an issue for just one proposal and we should close this issue as it no longer really pertains to this proposal alone. I would really appreciate it if you can recommend me a place where I can bring up the topic of enforcing a philosophy that I believe developers like littledan and I feel are necessary to have when considering adding something to the core JS language.

[Skillz4Killz]

@ljharb @bmeck Hey 👋 could you guys please recommend me where to bring up this discussion in the proper repo, please.

I think perhaps this is not an issue for just one proposal and we should close this issue as it no longer really pertains to this proposal alone. I would really appreciate it if you can recommend me a place where I can bring up the topic of enforcing a philosophy that I believe developers like littledan and I feel are necessary to have when considering adding something to the core JS language.

[bmeck]

@Skillz4Killz likely it would be directly contacting TC39/members, I can't think of a single place.

[hax]

I think this issue should be keep open. I have similar feeling like @Skillz4Killz , the essential issue is whether we really need a much high-level operation like upsert, does the cost of learning it add real values for average programmers?

[ljharb]

How would you propose to measure whether the cost of learning a thing is worth the value? A number of us think it is, and clearly some others think it isn’t. How do you suggest we make a decision based on whether or not it’s “worth it”?

[friendlyanon]

The only person who mentioned GC concerns was @bmeck however I fail to see how only entryView would cause issues there. Map is also a mutable map with no real way to make it immutable, I don't see how a mutable entry view would be an issue.

// upsert
function groupBy(iterable, key) {
  const mapper = typeof key === "function" ? key : x => x[key];
  const map = new Map();

  for (const value of iterable) {
    const mappedKey = mapper(value);
    // function for inserting is re-created with each iteration
    // yes, you could lift it out of the loop, but that's not always feasible
    // for the updater function someone might also use an identity function
    // despite nullish values acting like identity function arguments already
    map.upsert(mappedKey, null, () => []).push(value);
  }

  return map;
}

// entry view
function groupBy(iterable, key) {
  const mapper = typeof key === "function" ? key : x => x[key];
  const map = new Map();

  for (const value of iterable) {
    const mappedKey = mapper(value);
    // entry view object is also created with each iteration
    const entry = map.entryView(mappedKey);
    (entry.value ??= []).push(value);
  }

  return map;
}

[bmeck]

I've moved feedback from several issues to a revised design in https://github.com/tc39/proposal-upsert/issues/21

[resynth1943]

Thanks, @bmeck!