RFC: upsertItem state operator API

[markwhitfeld]

It has been proposed that an upsertItem state operator is added to the standard NGXS state operators.
PR #1795 proposes an approach, but I think we need to be more deliberate with the API.

It is based on my comment here: #926 (comment)
(but potentially over-engineered by me at the time)
But there is another simpler operator proposed by @marcjulian here: #926 (comment)

We really need to think about scenarios that are best handled by this operator and which would be better handled by another operator (instead of overloading this one too much).

Some scenarios in this problem space that we can think about how we will handle:

Item exists

1. Replace item if it matches the provided predicate
2. Replace item if it matches the provided index
3. Patch item if it matches the provided predicate
4. Patch item if it matches the provided index
5. Apply a state operator to item if it matches the provided predicate
6. Apply a state operator to item if it matches the provided index

Item doesn't exist

10. Prepend item if no items match the provided predicate
11. Append item if no items match the provided predicate
12. Insert item at a specified index if no items match the provided predicate
13. Insert item at a position determined by a function if no items match the provided predicate (think adding item to a sorted list... IMO this should not be part of this operator, but worth mentioning to be thorough)
14. Insert item at provided index which is one past the last index of the array (array grows by 1)
15. Insert item at provided index which is a more than one past the last index of the array (array grows to new index length)

Numbered items for reference in the discussion, a gap is left in case items are added later.
If you think of another scenario, then please mention it here.

[markwhitfeld]

An example of a proposed API:
(A critique of the one I put forward here - #926 (comment) )

export function upsertItem<T>(
  selector: number | Predicate<T>,
  upsertValue: T
);

• Functional design
  • Inserts or replaces items only.
  • No patching or advanced operations on the existing item.
  • If a predicate is used and not matched, then we append.
  • If an index is used, then the provided item will be found at that index, no matter how far past the end it is.
• Can handle scenarios: 1,2,11,14,15
• Cannot handle scenarios: 3,4,5,6,10,12,13
• Drawbacks:
  • The signature alone does not make it clear if an existing item would be replaced or patched.
  • Append is assumed when you use a predicate and it isn't matched.
  • No control over where the new item is added if you use a predicate.
  • The equivalent to all index-based usages could be done though using patch if patch supported updating arrays (it doesn't at the moment).
    • Example: patch({ [33]: 'my item' })

Please note that if you propose an API, it is good to identify the gaps as well as the advantages (as demonstrated here).

[joaqcid]

I agree that the API is not descriptive enough as to what the output will be, i do think though, that given we already have updateItem and insertItem as part of the operators family, using the same functionality they both provide on their own and combining them in a single operator should be sufficient.

updateItem currently handles 1, 2, 5, 6

insertItem currently handles 11, 14, 15

i think the rest of functionality should not became part of this operator, and rather become part of insertItem and updateItem or new operators to keep operators consistent.

that said, i think we should support operator for the scenario an item already exists, and in case item doesn't exist throw error?

export function upsertItem<T>(
  selector: number | Predicate<T>,
  upsertValueOrOperator: T
);

[markwhitfeld]

My hesitation with this signature is that in the case of the item not existing, then an operator doesn't make sense.
Would the state operator then be invoked with a null value for a new item?

Possibly we could have another parameter for the inserted value, or a parameters object with clearly labelled options.

[markwhitfeld]

Following this comment: #1796 (reply in thread)
Here is a proposal that covers all options (that we want to) with a parameters object approach.
I haven't done a full analysis of this, but thought I'd put it out there to see what people think...

The signature would be this:

export function upsertItem<T>(
  selector: number | ((item: T, index: number) => boolean),
  upsertOptions: UpsertOptions<T>
): StateOperator<T[]>;
export function upsertItem<T>(
  selector: number | ((item: T, index: number) => boolean),
  upsertOptions: UpdateOptions<T> & InsertOptions<T>
): StateOperator<T[]>;
// export function upsertItem<T>( /*...*/): StateOperator<T[]> { /* ...implementation */ }

So, you would either use just the UpsertOptions signature, or you would use a combination of one UpdateOption and one InsertOption. They are as follows:

interface AllUpsertOptions<T> {
  value: T;
}
interface AllInsertOptions<T> {
  prependNewItem: T;
  appendNewItem: T;
  // Debatable if this option should be included (see comment):
  customInsert: StateOperator<T[]>;
}
interface AllUpdateOptions<T> {
  replaceItem: T;
  updateItem: StateOperator<T>;
}

type InsertOptions<T> = MutuallyExclusiveProps<AllInsertOptions<T>>;
type UpdateOptions<T> = MutuallyExclusiveProps<AllUpdateOptions<T>>;
type UpsertOptions<T> = AllUpsertOptions<T>;

Here is a playground I used to flesh this out: https://stackblitz.com/edit/typescript-ngxs-upsert-item-proposal-1?file=index.ts
(You can see the type helpers I made there)

Usage best explained through examples:

/** By default this replaces or inserts the item at the provided index */
upsertItem<string>(2, { value: 'UPDATED' });
// 2. [Exists] Replace item if it matches the provided index
//   ['zero', 'one', 'two'] => ['zero', 'one', 'UPDATED']
// 14. [Doesn't exist] Insert item at provided index which is one past the last index of the array (array grows by 1)
//   ['zero', 'one'] => ['zero', 'one', 'NEW']
// 15. [Doesn't exist] Insert item at provided index which is a more than one past the last index of the array (array grows to new index length)
//   ['zero'] => ['zero', undefined, 'NEW']

/** By default this replaces or appends the item */
upsertItem<string>((item) => item === 'two', { value: 'UPDATED' });
// 1. [Exists] Replace item if it matches the provided predicate
//   ['zero', 'one', 'two'] => ['zero', 'one', 'UPDATED']
// 11. [Doesn't exist] Append item if no items match the provided predicate
//   ['zero', 'one'] => ['zero', 'one', 'NEW']
// 11. [Doesn't exist] Append item if no items match the provided predicate
//   ['zero'] => ['zero', 'NEW'

upsertItem<string>(2, { replaceItem: 'UPDATED', appendNewItem: 'NEW' });
// 2. [Exists] Replace item if it matches the provided index
//   ['zero', 'one', 'two'] => ['zero', 'one', 'UPDATED']
// 14. [Doesn't exist] Insert item at provided index which is one past the last index of the array (array grows by 1)
//   ['zero', 'one'] => ['zero', 'one', 'NEW']
// 15. [Doesn't exist] Insert item at provided index which is a more than one past the last index of the array (array grows to new index length)
//   ['zero'] => ['zero', undefined, 'NEW']

upsertItem<string>((item) => item === 'two', {
  replaceItem: 'UPDATED',
  appendNewItem: 'NEW',
});
// 1. [Exists] Replace item if it matches the provided predicate
//   ['zero', 'one', 'two'] => ['zero', 'one', 'UPDATED']
// 11. [Doesn't exist] Append item if no items match the provided predicate
//   ['zero', 'one'] => ['zero', 'one', 'NEW']
// 11. [Doesn't exist] Append item if no items match the provided predicate
//   ['zero'] => ['zero', 'NEW']

upsertItem<string>((item) => item === 'two', {
  replaceItem: 'UPDATED',
  prependNewItem: 'NEW',
});
// 1. [Exists] Replace item if it matches the provided predicate
//   ['zero', 'one', 'two'] => ['zero', 'one', 'UPDATED']
// 10. [Doesn't exist] Prepend item if no items match the provided predicate
//   ['zero', 'one'] => ['NEW', 'zero', 'one']
// 10. [Doesn't exist] Prepend item if no items match the provided predicate
//   ['zero'] => ['NEW', 'zero']

upsertItem<string>(2, {
  updateItem: (val) => `${val} UPDATED`,
  appendNewItem: 'NEW',
});
// 4. [Exists] Patch item if it matches the provided index
// 6. [Exists] Apply a state operator to item if it matches the provided index
//   ['zero', 'one', 'two'] => ['zero', 'one', 'two UPDATED']
// 11. [Doesn't exist] Append item if no items match the provided predicate
//   ['zero', 'one'] => ['zero', 'one', 'NEW']
// 11. [Doesn't exist] Append item if no items match the provided predicate
//   ['zero'] => ['zero', 'NEW']

upsertItem<string>(2, {
  updateItem: (val) => `${val} UPDATED`,
  prependNewItem: 'NEW',
});
// 4. [Exists] Patch item if it matches the provided index
// 6. [Exists] Apply a state operator to item if it matches the provided index
//   ['zero', 'one', 'two'] => ['zero', 'one', 'two UPDATED']
// 10. [Doesn't exist] Prepend item if no items match the provided predicate
//   ['zero', 'one'] => ['NEW', 'zero', 'one']
// 10. [Doesn't exist] Prepend item if no items match the provided predicate
//   ['zero'] => ['NEW', 'zero']

upsertItem<string>((item) => item === 'two', {
  updateItem: (val) => `${val} UPDATED`,
  appendNewItem: 'NEW',
});
// 3. [Exists] Patch item if it matches the provided predicate
// 5. [Exists] Apply a state operator to item if it matches the provided predicate
//   ['zero', 'one', 'two'] => ['zero', 'one', 'two UPDATED']
// 11. [Doesn't exist] Append item if no items match the provided predicate
//   ['zero', 'one'] => ['zero', 'one', 'NEW']
// 11. [Doesn't exist] Append item if no items match the provided predicate
//   ['zero'] => ['zero', 'NEW']

/* 
I am in two minds if the `customInsert` option should be part of the operator
If we don't include it then we dont support the following scenarios:
// 12. [Doesn't exist] Insert item at a specified index if no items match the provided predicate
// 13. [Doesn't exist] Insert item at a position determined by a function if no items match the provided predicate (think adding item to a sorted list... IMO this should not be part of this operator, but worth mentioning to be thorough)

I am ok to not support these to be honest, because it is a really specific requirement as opposed to general purpose

That being said, if we were to include that operator then these scenarios can be handled like this:
*/

upsertItem<string>((item) => item === 'two', {
  replaceItem: 'UPDATED',
  customInsert: insertItem('NEW', 1),
});
// 1. [Exists] Replace item if it matches the provided predicate
//   ['zero', 'one', 'two'] => ['zero', 'one', 'UPDATED']
// 12. [Doesn't exist] Insert item at a specified index if no items match the provided predicate
//   ['zero', 'one'] => ['zero', 'one', 'NEW']
// 12. [Doesn't exist] Insert item at a specified index if no items match the provided predicate
//   ['zero'] => ['zero', 'NEW']

upsertItem<string>((item) => item === 'c', {
  replaceItem: 'UPDATED',
  customInsert: (val) => {
    const i = val.findIndex((item) => item > 'c');
    if (i < 0) {
      return [...val, 'NEW'];
    }
    return [...val.slice(0, i), 'NEW', ...val.slice(i)];
  },
});
// 1. [Exists] Replace item if it matches the provided predicate
//   ['a', 'b', 'c', 'd'] => ['a', 'b', 'UPDATED', 'd']
// 13. [Doesn't exist] Insert item at a position determined by a function if no items match the provided predicate
//   ['a', 'b', 'd'] => ['a', 'b', 'NEW', 'd']
// 13. [Doesn't exist] Insert item at a position determined by a function if no items match the provided predicate
//   ['a', 'b'] => ['a', 'b', 'NEW']

Thoughts?

[markwhitfeld]

cc @joaqcid @splincode @Carniatto @arturovt @poloagustin @kuncevic @marcjulian @troydietz @beaussan

[joaqcid]

IMO this API is too much for an upsert operation, i would strive for simplicity here, even if we are not handling some scenarios.

i'd trade off only allowing value and not an operator to keep the operator simpler...

[splincode]

this API is too much for an upsert operation agree!

[markwhitfeld]

Maybe the question should be, what are the common use case(s) that should be covered out of the box?
Then we work on an API from there...

[joaqcid]

right, well, from my point of view upsert refers to updating an item if exists or inserting it if it doesnt,

since we already have updateItem and insertItem, what would make sense to me is to combine those two into a single one,

i see combining these is not straight forward, given updateItem supports an operator as param which for insertItem wont work.

but in terms of scenarios i wouldn't go further than what updateItem and insertItem already support (i.e. insertItem doesnt have prepend or append options)

i cant make my mind if i prefer to drop operator support as param, but i cant think of how we could handle an operator in an insertItem scenario