tc39 / proposal-change-array-by-copy

Provides additional methods on Array.prototype and TypedArray.prototype to enable changes on the array by returning a new copy of it with the change.
https://tc39.es/proposal-change-array-by-copy/
511 stars 20 forks source link

Alternative to `toSpliced()` #80

Open hax opened 2 years ago

hax commented 2 years ago

Sorry it's too late to add this issue, I commented that on the yesterday meeting. Though I didn't want to block this proposal in the meeting, I think we still have chance to think about the alternative.

The problems of splice, toSpliced

There are many complaints about the old splice method in the community, the main issues of it are:

toSpliced() inherit all these warts, and might worse because it return semantics differ from splice, this means if you would see code like:

let originalSlice = a.slice(start, end)
let newArray = a.toSpliced(start, end - start, ...items)

Not easy to understand and maintain, and it's even have random bug when start or end are negative!

Alternative

Do not add toSpliced but add:

a.sliceReplace(start, end, items) a.withSliceReplaced(start, end, items)

which follow the convention of slice, solve the problems.

Note, this could be a separate proposal, but if we had withSliceReplaced(start, end, items), it's no need to add a bad api like toSpliced anymore.

shaozj commented 2 years ago

these apis are too long

zloirock commented 2 years ago

For me, .toSpliced is the only useful part of this proposal. The name is fine. Yes, the method signature is complex - but I don't think that alternative options are better.

hax commented 2 years ago

@shaozj The name could be revised if u have other options.

@zloirock I already give some concrete reasons why splice api is bad, could u share your thought about why u think the alternative is not better? Thank u.

zloirock commented 2 years ago

I already give some concrete reasons why splice api is bad

Subjective reasons without any argumentation:

Bad api: It's so hard to figure out what the code like a.splice(1, 2, 3, 4, 5) really do.

We could say the same about any method that takes more than 2 argument and used not too often. You options does not looks better.

The signature of .splice is already well-known, you wanna enforce users to remember one more signature pattern?

a.sliceReplace(start, end, items)

end vs deleteCount

I check code base of many my projects - in most (~90%) cases, I know the number of arguments that should be removed in .splice (fixed number or number of elements that should be added), in case of the second argument-end position I will be forced to calculate it.

items vs ...items

In most cases, items are just some variables. I remember only some cases where it was required to add elements from a collection with various number of elements. In case of inserting just one value, I will be forced to wrap it to array brackets - why?


.splice was unpopular method, in most cases it was used where it was better to use something else - but only because this method worked in place - .toSplice fixed all it's problems. The problem of signature of this method is just that it should accept many arguments - but your options does not fix it at all.

rricard commented 2 years ago

During the plenary we brought up 2 points for toSpliced:

For those reasons, we don't intend to drop toSpliced from this proposal regardless of the perceived ease of use of its API (or lack thereof). This does not prevent a follow-on proposal however to propose different APIs that would be more ergonomic: the good news is that you could probably polyfill those new APIs using toSpliced as well!

hax commented 2 years ago

@zloirock

We could say the same about any method that takes more than 2 argument and used not too often. You options does not looks better.

Too many arguments could be the problem, but in this specific case, it's not just because of too many args, but the mixing of the position/length params and inserted items. Compare a.foo(1, 2, 3, 4, 5) and a.foo(1, 2, [3, 4, 5]), u could see they have very different readability.

The signature of .splice is already well-known, you wanna enforce users to remember one more signature pattern?

It's too easy to say "well-known", u could say any ES5 era methods are well-known, but does people really could use splice without reading manual everytime? I know you are a very good programmer and maybe you could remember all of them, but what I heard from the average programmers are very different.

Note, my suggestion is not inventing a new thing, but follow the real well-known method slice and using a much easy mental model: replace a slice (items) of an array with a new items.

a.sliceReplace(start, end, items)

end vs deleteCount

I check code base of many my projects - in most (~90%) cases, I know the number of arguments that should be removed in .splice (fixed number or number of elements that should be added), in case of the second argument-end position I will be forced to calculate it.

I think this is a good argument.

Which API is better, (start, end) or (start, length), this is a question. I'm neutral on that, but two very similar api choose different style is definitely a problem. For example, in the old days of JS, most people can never figure out the difference of s.slice/substr/substring, and I believe most people now only use slice because it have the consistency on all indexed collections (string, array, typed array...)

Consider slice as the most common used api in all indexed collections, I think follow the api of it would be a better choice.

About calculation, u may need calc in either case. One important point is, a.withSliceReplaced(start, start+len) is always correct even start is negative, but a.toSpliced(start, end-start) is not if start/end have different sign.

items vs ...items

In most cases, items are just some variables. I remember only some cases where it was required to add elements from a collection with various number of elements. In case of inserting just one value, I will be forced to wrap it to array brackets - why?

The reason is sliceReplace/withSliceReplaced are based on the concept of slice, the api is used to replace a slice (from start to end) with a new slice (items).

And array bracket [] also give a visual boundary which solve a.splice(1,2,3,4) problem I mentioned.

Of coz, if the case is inserting just one value, it seems not very optimized. But I assume the common case of "inserting one value" is just replace one element, so instead of toSpliced(i, 1, value), we should use with(i, value). And the general cases of removing any number of items and only insert one seems already a special case, withSliceReplaced(start, end, [happenToOnlySingleItem]) could even make it much clear. On the other hand, when I do code review I see splice(start, len, array) and always worry does the author really mean to add single array value, not typo of ...array.


Subjective reasons without any argumentation:

I hope we could at least have some agreement about how we could discuss api design problem, thank you.

ljharb commented 2 years ago

splice's signature is not well-known at all; it's wildly complex. Even setting aside those who accidentally add the "p" and intend slice, community members are consistently and often confused about how to use splice properly. splice is horrific and shouldn't be used as an inspiration for anything; toSpliced is the lone example that would ever be worth including.

zloirock commented 2 years ago

@ljharb if a such mehod is too complex for you and you confuse it with .splice - it does not mean that it's a problem of all. For me it's the most logical and easily memorized signature for a such method.

zloirock commented 2 years ago

@hax you position makes sense, but I agree with @rricard from the comment above. This proposal just provide non-mutating equals of already existent methods, with the same naming changed by the pattern and signatures, so I think that .toSpliced should be added anyway, but if you think that required a method with another name and signature - it can be done in another proposal.

ljharb commented 2 years ago

@zloirock sure. but all of my experience working with engineers of all skill levels over my entire career tells me that "splice makes sense to me" is the extreme minority case.

zloirock commented 2 years ago

Awesome, so instead of any argumentation why it's so "horrific", you wrote something about the skill level... My argumentation above. And why do you think that my skill and experience level is worse than your? -)

ljharb commented 2 years ago

I think I'm not making myself clear, apologies.

I'm saying that, while it's great that you find it clear, I have seen a great many people ranging from "brand new engineers" to "extreme expert engineers, much more qualified than myself" find the splice API confusing - in other words, I've seen that skill level/experience has no bearing on the likelihood one will understand how splice works.

Thus, my experience there suggests that you are in a niche category of people who find it clear. That may not be accurate, of course, but it's what my experience suggests.

zloirock commented 2 years ago

Clear and reasonable. Anyway, my position above.

acutmore commented 2 years ago

Note, my suggestion is not inventing a new thing, but follow the real well-known method slice and using a much easy mental model: replace a slice (items) of an array with a new items.

Hi @hax

Where I think using the slice term makes less sense is when only inserting new items. The mental model for withSliceReplaced is to select a zero-length slice of the array and then replace that with a longer slice. Which, at least to me, seems more confusing than the toSpliced model of delete N and insert N.

const newData = getNewData();
return currentList.toSpliced(/* at: */ index, /* delete: */ 0, ...newData);

vs

const newData = getNewData();
return currentList.withSliceReplaced(/* start: */ index, /* end: */ index, ...newData);
theScottyJam commented 2 years ago

I very much dislike the splice method as well, and find its API to be very unintuitive. My issue with it is that it's a jack-of-all-trades (and it's poorly named, and it's unclear which arguments do what). You can use it to remove elements from the middle of an array, or to insert elements into the middle, or a combination of the two actions at the same time. I'd rather just have a separate function to do these two actions (and yes, I know there's performance concerns with making them separate functions, but if I was really worried about performance in the particular place I'm coding, I would use the in-place modification functions).

It might be too late to really add an improved .spliced() to this proposal, but I would be happy if we just riped out .withSpliced() from it, and started a new proposal to figure out the best way to supersede its functionality.

hax commented 2 years ago

As #88 , TypedArray.p.toSpliced also make the problem worse: it's either increase inconsistency in TypedArray (has toSpliced but no splice), or inconsistency between TypedArray and Array/Tuple (only have Array/Tuple.p.toSpliced but no TypedArray.p.toSpliced).

acutmore commented 2 years ago

Putting toSpliced aside here is the cross-sections of the 3 interfaces (Array, TypedArray (TA), and Tuple).

Array.prototype = Core-1 + Core-2 + Array-1 + Array-2 TA.prototype = Core-1 + Core-2 + TA Tuple.prototype = Core-1 + Array-1


Core-1 (non-mutating):

Core-2 (mutating):

Array-1 (non-mutating):

Array-2 (mutating):

TA:

ljharb commented 2 years ago

So based on that nomenclature, we're discussing whether toSpliced is in "Core-1" vs "Array-1"?

acutmore commented 2 years ago

In #88 yes (maybe I should have posted my comment there instead).

It was more to help me visualise @hax 's comment.

As https://github.com/tc39/proposal-change-array-by-copy/issues/88 , TypedArray.p.toSpliced also make the problem worse: it's either increase inconsistency in TypedArray (has toSpliced but no splice), or inconsistency between TypedArray and Array/Tuple (only have Array/Tuple.p.toSpliced but no TypedArray.p.toSpliced).

"has toSpliced but no splice" - My feeling is that there is a consistency here. TypedArrays have no methods that can impact length (push, pop, shift unshift). This does not apply to toSpliced.

"inconsistency between TypedArray and Array/Tuple" - there is some president for this. Only TypedArrays have set, and only Arrays have concat and flatMap.

So I don't think it's 100% clear what would be the most consistent API here.