Changes to ReactiveArray
Opened this issue · 2 comments
reverse and sort don't make a whole lot of sense as mutable methods on ReactiveArrays, and it would make more sense for them to return new arrays instead. With that in mind, I plan to add the following methods from the change Array by copy proposal:
withReversedwithSorted- are the others in the linked proposal needed? Are there some other methods that should be added?
…and remove reverse and sort. If someone needs the mutating version, they could simply do arr.value = arr.value.reverse() for example.
Additionally, there are some other methods on native Arrays that don't make a whole lot of sense for ReactiveArrays with correct usage of the library, or are impossible to have their updates be optimized. With that in mind, I plan to remove flat, flatMap, copyWithin, and fill.
concat is somewhat useful, but the semantics of native Array.prototype.concat() are quite convoluted, and I would much rather have a simple function that does nothing but concatenate ReactiveArrays. I don't know if it would make more sense to change concat to have semantics that are not in line with the native equivalent, or to remove concat and make a new function with simpler semantics to avoid confusion.
Finally, it seems like the mutating methods proposed and discussed in #4 haven't been very popular, and similar functionality could be achieved with similar effort using a for loop, so I plan to remove the ones implemented: mutFilter and mutMap.
Click for a list of `ReactiveArray` methods
On ReadonlyReactiveArray:
bind❇clone❇concat🚮?entieseveryexclusiveSome❇filterfindfindIndexflat🚮flatMap🚮forEachgetincludesindexOfjoinkeyslastIndexOfmappipe❇reducereduceRightslicesometoJSONunbind❇update❇valueswithReversed🆕withSorted🆕
On ReactiveArray:
copyWithin🚮fill🚮mutFilter❇🚮mutMap❇🚮poppushreverse🚮set❇shiftsort🚮spliceunshift
🆕 = proposed addition
❇ = not present on native Arrays
🚮 = planned for removal
Thoughts?
Is there a reason you'd want to have a sort and reverse method that can be mutable? Why not just ReactiveArray inherit/implement Array.prototype.sort and Array.prototype.reverse?
Though i'm slightly confused by:
With that in mind, I plan to add the following methods from the change Array by copy proposal:
with:
If someone needs the mutating version, they could simply do arr.value = arr.value.reverse() for example.
Aren't these contradicting? The proposal adds those methods that make the values mutable, yet you also don't want a mutating version built into destiny?
Though maybe it isn't as simple as "oh ReactiveArray just inherits sort" due to Reactivity (I'm aware i'm not fully 100% understanding of how these things are build within destiny)
In terms of methods you supply that aren't present on native arrays, maybe it's an idea to only provide non-native methods that are specific to destiny, for example, bind and pipe, these are purely destiny functionality essentially, whereas exclusiveSome (and maybe set, clone and update? Haven't looked into those methods yet though) seem more like 'non native polyfills'. But you seem to somewhat taking this approach anyways, with the idea of removing mut* methods (which makes a lot of sense to me at last, to keep everything as native as possible)
Is there a reason you'd want to have a sort and reverse method that can be mutable? Why not just
ReactiveArrayinherit/implementArray.prototype.sortandArray.prototype.reverse?
Though maybe it isn't as simple as "oh ReactiveArray just inherits sort" due to Reactivity (I'm aware i'm not fully 100% understanding of how these things are build within destiny)
ReactiveArray doesn't directly extend Array at all. Most methods are reimplemented for the class to mimic the native methods as closely as feasible. However, some methods that are completely infeasible to be reimplemented access the underlying array, call the method on it, and then rewrites all the current values with whatever the native one returns with:
reverse (): this {
this.value = this.value.reverse();
return this;
}Note that setting this.value will not optimize what's going on with the class, and will instead remove everything it has currently and add everything that's being added, which is not a particularly fast operation:
set value (
items: ReadonlyArray<InputType | TArrayValueType<InputType>>,
) {
this.#splice(
0,
this.#value.length,
...items,
);
}Though i'm slightly confused by:
With that in mind, I plan to add the following methods from the change Array by copy proposal:
with:
If someone needs the mutating version, they could simply do arr.value = arr.value.reverse() for example.
Aren't these contradicting? The proposal adds those methods that make the values mutable, yet you also don't want a mutating version built into destiny?
Array.prototype.sort and reverse mutate the current array instead of returning a new one. All the methods linked in the proposal change the array by copy, leaving the original array untouched.
In terms of methods you supply that aren't present on native arrays, maybe it's an idea to only provide non-native methods that are specific to destiny, for example,
bindandpipe, these are purely destiny functionality essentially, whereasexclusiveSome(and maybeset,cloneandupdate? Haven't looked into those methods yet though) seem more like 'non native polyfills'. But you seem to somewhat taking this approach anyways, with the idea of removingmut*methods (which makes a lot of sense to me at last, to keep everything as native as possible)
You're right about exclusiveSome. It would remain to be the only one that's "more like a 'non native polyfill'", like you put it — that is, if we don't remove it as well.
setsets a value on the array.arr.set(0, value)is a replacement forarr[0] = valuebecause ReactiveArrays don't have bracketed index access (anymore).clone()is technically also a "non-native polyfill", but because none of the normal ways of cloning the array (like.slice(0)) are a good idea for cloning a ReactiveArray, I wanted to include it to have a standard way of efficiently cloning the array, seeing as it is a common operation and one people try to do in inadvisable ways in the absense of this method.updateis used for forcing change events to be dispatched. This is necessary with the way update propagation is currently implemented. Depending on the success I have with the update propagation rework, this may not be necessary in the future. But for the time being, it is needed.