Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add delete method #23

Open
George-Payne opened this issue May 19, 2020 · 3 comments
Open

Add delete method #23

George-Payne opened this issue May 19, 2020 · 3 comments
Labels
Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through.

Comments

@George-Payne
Copy link

George-Payne commented May 19, 2020

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.

@Serabe
Copy link
Contributor

Serabe commented May 19, 2020

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

@Serabe
Copy link
Contributor

Serabe commented May 19, 2020

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 #24 (comment))

@Serabe
Copy link
Contributor

Serabe commented May 19, 2020

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

@splitinfinities splitinfinities added Resolution: Needs Documentation Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through. labels Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Refine This PR is marked for Jira refinement. We're not working on it - we're talking it through.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants