Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Alternative to toSpliced() #80

Open
hax opened this issue Mar 31, 2022 · 19 comments
Open

Alternative to toSpliced() #80

hax opened this issue Mar 31, 2022 · 19 comments

Comments

@hax
Copy link
Member

hax commented Mar 31, 2022

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:

  • Bad name: splice is a very uncommon word, as word usage frequency from COCA (Corpus of Contemporary American English) , "splice" is at the position of 20171, and as this comment, even native speakers might not know the meaning of that word anymore.
  • Bad api: It's so hard to figure out what the code like a.splice(1, 2, 3, 4, 5) really do.
  • Combination of the bad name and api: the name of splice looks very like slice, so those who don't know the word well would guess it's a mutation version of slice, but it not follow slice api.

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
Copy link

shaozj commented Mar 31, 2022

these apis are too long

@zloirock
Copy link
Contributor

zloirock commented Mar 31, 2022

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
Copy link
Member Author

hax commented Mar 31, 2022

@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
Copy link
Contributor

zloirock commented Mar 31, 2022

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
Copy link
Member

rricard commented Mar 31, 2022

During the plenary we brought up 2 points for toSpliced:

  • The goal of this proposal is to forge non-mutating APIs out of existing mutating APIs; this wasn't re-motivated during this week's presentation but you can check out previous presentations to see our rationale here:
  • After the committee asked us to reduce the number of ported methods, toSpliced became needed to cover things such as non-mutating deletion at a given index, etc..

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
Copy link
Member Author

hax commented Mar 31, 2022

@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
Copy link
Member

ljharb commented Mar 31, 2022

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
Copy link
Contributor

@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
Copy link
Contributor

zloirock commented Mar 31, 2022

@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
Copy link
Member

ljharb commented Mar 31, 2022

@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
Copy link
Contributor

zloirock commented Mar 31, 2022

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
Copy link
Member

ljharb commented Mar 31, 2022

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
Copy link
Contributor

Clear and reasonable. Anyway, my position above.

@acutmore
Copy link
Collaborator

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
Copy link

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
Copy link
Member Author

hax commented Jun 24, 2022

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
Copy link
Collaborator

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):

  • at
  • entries
  • every
  • filter
  • find
  • findIndex
  • findLast
  • findLastIndex
  • flat
  • flatMap
  • forEach
  • includes
  • indexOf
  • join
  • keys
  • lastIndexOf
  • map
  • reduce
  • reduceRight
  • slice
  • some
  • toLocaleString
  • toString
  • values
  • toSorted (TBC)
  • toRevered (TBC)
  • with (TBC)

Core-2 (mutating):

  • copyWithin
  • fill
  • reverse
  • sort

Array-1 (non-mutating):

  • concat
  • flat
  • flatMap

Array-2 (mutating):

  • pop
  • push
  • shift
  • splice
  • unshift

TA:

  • set
  • subarray

@ljharb
Copy link
Member

ljharb commented Jun 24, 2022

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

@acutmore
Copy link
Collaborator

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

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

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).

"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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants