Thoughts And Concerns Of Effects of Proposal #14
Comments
|
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, 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, |
|
"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. |
|
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. |
|
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. 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 If you're opposed to convenience methods, |
|
I am also totally open to name changes for |
|
If anything
|
|
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
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.
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.
Why are we adding this to the core language? |
|
(I think you'll find the majority of lodash is already in the language) |
|
@ljharb If majority of lodash is already in the language (actually I agree with this point) , why people always need lodash ? 😂 |
|
@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') // 0Basically, for 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. |
|
Exposing a mutable object whose mutations affect the Map does not seem simple or desirable to me (if I’m understanding your suggestion correctly). |
Yes this is what
"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,
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 |
|
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.
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 fnCallsThis 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.
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.
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. |
|
@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.
Same. I also love the minimal simplicity.
@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.
@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 // 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
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 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 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.
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.
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: 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. :) |
Yes, the 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 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();
}
}
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
I don't think any other APIs are naming their parameters in this way. See things like
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
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.
You don't have to use it! We are not planning to break any existing APIs :).
But then if you need to add either Even if these are added you do end up with that boilerplate about 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 function add2(name) {
const existing = map.get(name);
if (!existing) map.set(name, 0);
map.addTwo(name);
}If your map had an |
|
In your example, you use the following code to illustrate your point:
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? |
|
In your example
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. |
|
@bmeck Ahh, thanks for the clarification, had a brain-freeze moment. |
|
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.
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.
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.
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.
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.
In this case, I would simply not have created a |
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.
We actually had a presentation at TC39 this month about how
We are open towards any recommendations on allowing this API to be easier to understand :).
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.
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.
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. |
|
@Skillz4Killz it's not simpler - you had to repeat |
I would be very shocked if someone had a falsy value for a map holding
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.
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.
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. tc39/proposal-array-filtering#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. |
|
@ljharb @bmeck Hey 👋 could you guys please recommend me where to bring up this discussion in the proper repo, please.
|
|
@Skillz4Killz likely it would be directly contacting TC39/members, I can't think of a single place. |
|
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 |
|
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”? |
|
The only person who mentioned GC concerns was @bmeck however I fail to see how only // 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;
} |
|
I've moved feedback from several issues to a revised design in #21 |
|
Thanks, @bmeck! |
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
.forEachand 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.
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 :)
The text was updated successfully, but these errors were encountered: