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

To change: Reduction of scope #27

Closed
rricard opened this issue May 6, 2021 · 10 comments · Fixed by #41
Closed

To change: Reduction of scope #27

rricard opened this issue May 6, 2021 · 10 comments · Fixed by #41

Comments

@rricard
Copy link
Member

rricard commented May 6, 2021

This issue is a consolidation of #24 by @benlesh (thanks for opening the discussion on this!)


This thread intends to propose the following reduction of scope. This has been discussed in the last Record & Tuple meeting but has not been validated, pending the definition fo the exact scope and further discussion.

-   Array.prototype.filled(value, start, end) -> Array
-   Array.prototype.copiedWithin(copiedTarget, start, end) -> Array
-   Array.prototype.popped() -> Array
-   Array.prototype.pushed(values...) -> Array
+   Array.prototype.reversed() -> Array
-   Array.prototype.shifted() -> Array
+   Array.prototype.sorted(compareFn) -> Array
+   Array.prototype.spliced(start, deleteCount, ...items) -> Array
-   Array.prototype.unshifted(...values) -> Array
+   Array.prototype.with(index, value) -> Array

# Key:
-   to be removed
+   will be kept

updated to reflect our current intent to change (See #27 (comment) )

The rationale behind this proposed change is that those functions could be replaced with slice & concatenation techniques that would be as explicit. It would also prevent unwanted over-copying of arrays, notably by preventing chaining of those operations. Finally it reduces web-compatibility risk.

The main objection would be that those methods have been designed around Tuples in the first place and could be useful there. As we want to keep both proposals in sync we would have to remove them from Tuples as well.

@rricard rricard added the question Further information is requested label May 6, 2021
@rricard rricard added this to the Stage 2 milestone May 6, 2021
@zloirock
Copy link
Contributor

zloirock commented May 6, 2021

How do you propose to replace .spliced? For me, it's the most useful method of this proposal.

@rricard
Copy link
Member Author

rricard commented May 6, 2021

Here is the example from mdn with spliced:

let months = ['Jan', 'March', 'April', 'June'];
months = months.spliced(1, 0, 'Feb');
// inserts at index 1
console.log(months);
// expected output: Array ["Jan", "Feb", "March", "April", "June"]

months = months.spliced(4, 1, 'May');
// replaces 1 element at index 4
console.log(months);
// expected output: Array ["Jan", "Feb", "March", "April", "May"]

Would become:

let months = ['Jan', 'March', 'April', 'June'];
months = [...months.slice(0, 1), "Feb", ...months.slice(1)];
// inserts at index 1
console.log(months);
// expected output: Array ["Jan", "Feb", "March", "April", "June"]

months =  [...months.slice(0, 4), "May", ...months.slice(5)];
// replaces 1 element at index 4
console.log(months);
// expected output: Array ["Jan", "Feb", "March", "April", "May"]

One thing to note: this form is the most "dumb" application of splice with this model, you can be actually more brief in a case by case basis. However, I am open to reintroduce spliced, it is originally linear time anyway so there is no drawback in bringing it back.

@zloirock
Copy link
Contributor

zloirock commented May 6, 2021

.slice() + .splice() is shorter and simpler than examples what do you propose, this proposal is designed to avoid cases like this. Sometimes we need to remove or add element(s) in the middle of the array - for example, a simple shopping cart - and .spliced here is the best option. I think that .spliced should be in this proposal.

@rricard
Copy link
Member Author

rricard commented May 6, 2021

That is a fair point, I proposed this as I know splice is not everyone favorite's method but as it does not go against the rationale presented, there is a good case for keeping it. Same would be said of fill and copyWithin, will put them as questionned.

@acutmore
Copy link
Collaborator

acutmore commented May 6, 2021

comparisons with their equivalents:

- arr.filled(value, start, end)
+ [...arr.slice(0, start), ...arr.slice(start, end).map(() => value), arr.slice(end)]

- arr.copiedWithin(target, start, end)
+ nonMutatingCopyWithin(arr, target, start, end); // implementation below

- arr.popped()
+ arr.slice(0, -1)

- arr.pushed(...items)
+ arr.concat([...items])

- arr.shifted()
+ arr.slice(1)

- arr.spliced(start, deleteCount, ...items)
+ [...arr.slice(0, start), ...items, ...arr.slice(start + deleteCount)]

- arr.unshifted(...items)
+ [...items, ...arr]
+ or arr.spliced(0, 0, ...items)

// extracted out as I couldn't find a one-liner that handled all paramerters
function nonMutatingCopyWithin(arr, relTarget, relStart = 0, relEnd = arr.length) {
    const length = arr.length;
    const target = relTarget >= 0 ? relTarget : length + relTarget;
    const start = relStart >= 0 ? relStart: length + relStart;
    const end = Math.min(relEnd >= 0 ? relEnd : length + relEnd, start + (length - target));

    return [
        ...arr.slice(0, target),
        ...arr.slice(start, end),
        ...arr.slice(target + (end - start))
    ];
}

please shout if there are mistakes, or cleaner alternatives

@zloirock
Copy link
Contributor

zloirock commented May 6, 2021

@rricard unlike .spliced, I don't see any .filled and .copiedWithin real-life use cases. A half of .splice problems are fixed in the .spliced.

@rricard
Copy link
Member Author

rricard commented May 6, 2021

Agreed, maybe they are actually more valuable on TypedArrays however so if that is the case we would keep them

@rricard rricard modified the milestones: Stage 2, Stage 3 May 21, 2021
@acutmore
Copy link
Collaborator

Another aspect to consider for methods that have syntactic equivalents, is that the syntax would be type dependent.
e.g.

  • [...list] shallow copy but returned value is always an Array. non generic
  • list.slice() shallow copy and returned type is based on the receiver. generic

So while a non-mutating version of unshift could be implemented as: [...newItems, ...original] we would lose if the original was an Array, TypedArray or Tuple. That said if we had a non-mutating splice then that could be an alternative that preserves the type. .withSpliced(0, 0, ...newItems).

@acutmore
Copy link
Collaborator

acutmore commented Aug 31, 2021

Implementors have expressed support in a reduction of scope to reduce the maintenance cost of adding 20 new methods (10 x 2, Array and TypedArray).

A reduced set of methods would look like:

  • non-mutating sort
  • non-mutating reverse
  • non-mutating splice
  • non-mutating index replacement e.g: .withAt(index, value). Note: this could be achieved with .nonMutatingSplice(index, 1, value) but is kept as Tuple does not have index assignment and we think it should have a clearer alternative than deleting and adding a value with splice.

Rationale behind dropped methods:

  • non-mutating-fill: Unless the receiver is holey .map can be used, though some math is required if using the additional 2nd and 3rd index arguments of fill.
  • non-mutating-copyWithin: Low usage as a mutator link. MDN describes the mutating methods as "a high-performance method to shift the data" link. Which may not apply in non-mutating scenarios.
  • non-mutating-pop: is .slice(0, -1)
  • non-mutating-push: is .concat([v])
  • non-mutating-shift: is .slice(1)
  • non-mutating-unshift: is .nonMutatingSplice(0, 0, ...vs)

cc: @syg @codehag @benlesh

@rricard rricard added polyfill readme spec and removed question Further information is requested labels Aug 31, 2021
@rricard rricard changed the title Proposed change: Reduction of scope To change: Reduction of scope Aug 31, 2021
@rricard
Copy link
Member Author

rricard commented Aug 31, 2021

This reduction of scope is scheduled to be done on both this proposal & the Tuple prototype in Record & Tuple. tc39/proposal-record-tuple#227 (comment)

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

Successfully merging a pull request may close this issue.

3 participants