stencil-store icon indicating copy to clipboard operation
stencil-store copied to clipboard

Add delete method

Open George-Payne opened this issue 4 years ago • 3 comments

Its not unusual to use a store as a map of items:

export interface Item {
    id: string;
    name: string;
    created: Date;
}

export type ItemStore = Record<string, Item>;

const itemStore = createStore<ItemStore>({});

This seems to work really well with @stencil/store:

const myItem = { id: 'a', name: 'My Item', created: new Date() };

itemStore.set(myItem.id, myItem);
// itemStore.state[myItem.id] = myItem;

itemStore.get('a') // => myItem

until you try to delete an item:

// nothing happens
delete itemStore['a'];

// sets the value to literal undefined
itemStore.set('a', undefined as any)
itemStore.state // { a: undefined } 

I'd like to propose adding a delete method to the store to allow you to do the following:

// deletes item
delete itemStore['a'];

// deletes the item from the store
itemStore.delete('a') // => boolean
itemStore.state // { } 

What are your opinions on this? I can see it being an issue with the current recommended usage:

export interface MyStore {
    clicks: number,
    seconds: number,
    squaredClicks: number
}

const { state, onChange } = createStore<MyStore>({
  clicks: 0,
  seconds: 0,
  squaredClicks: 0
}));

onChange('clicks', value => {
  state.squaredClicks = value ** 2;
});

// this is probably undesirable 
delete state.clicks;
console.log(state.squaredClicks) //=> NaN

But I think its utility outweighs its possible pitfalls.

George-Payne avatar May 19 '20 15:05 George-Payne

Can you add a situation where you are using the store in such a way?

Serabe avatar May 19 '20 17:05 Serabe

Just to make sure, I'm not opposed to adding this to ObservableMap for completeness, but I have some concerns with adding this to stores (see https://github.com/ionic-team/stencil-store/pull/24#issuecomment-630966664)

Serabe avatar May 19 '20 17:05 Serabe

I would suggest using the following pattern for these cases:

const createDataStore = <T>() => {
  const {state} = createStore({ values: [] as T[] });

  return {
    add(value: T) {
       state.values = state.values.concat(value);
    },
    delete(value: T) {
       state.values = state.values.filter(x => x !== value);
    },
    find(condition) {
       return state.values.find(condition);
    }
  }
}

export const DataStore = createDataStore();

Pros:

  1. Works right now.
  2. It does not create a lot of different keys for subscription.
  3. Everything is updated as long as any accessor methods (like find) reads from state.values.

Cons:

  1. When a change is done, all the components using the store are re-rendered (but, to be honest, I think we would need to do something similar in the end if we were to support delete).

Serabe avatar May 19 '20 17:05 Serabe