Use patch for list updates #1511
Conversation
lib/lists/actions.js
Outdated
| @@ -22,9 +22,27 @@ export function updateList(store, { listName, fn }) { | |||
| store.commit(LIST_LOAD_PENDING, { listName }); | |||
|
|
|||
| return getListFromAPI(prefix, listName) | |||
| // TODO check this error and make sure its a 404 before returning an empty array | |||
There was a problem hiding this comment.
thanks for this, clay swallowing errors has been a pain point x.x
| @@ -16,7 +16,7 @@ | |||
| <script> | |||
| import _ from 'lodash'; | |||
| import item from './autocomplete-item.vue'; | |||
| import { getProp, removeListItem } from '../lib/lists/helpers'; | |||
There was a problem hiding this comment.
removeLIstItem and addListItem are no longer used (and shouldn't be used) so they can be removed
| @@ -22,9 +22,27 @@ export function updateList(store, { listName, fn }) { | |||
| store.commit(LIST_LOAD_PENDING, { listName }); | |||
|
|
|||
| return getListFromAPI(prefix, listName) | |||
| // TODO check this error and make sure its a 404 before returning an empty array | |||
| .catch(() => []) // allow adding to a new list | |||
| .then(listItems => fn(_.cloneDeep(listItems))) | |||
| .then(listItems => saveList(prefix, listName, listItems)) | |||
There was a problem hiding this comment.
so it looks like the remaining lists using 'updateList' are new-pages and bookmarks, neither of which will ever reach a large number of items.
However if we keep updateList in the codebase, then
- future clay devs may introduce it to update a large list without realizing
- plugins may introduce bugs by dispatching an action for a large list.
so I could see value in forcing patchList to be used moving forward (i.e. remove updateList). But no strong opinions there
There was a problem hiding this comment.
yea i was hesitant to remove it from the codebase in case there were already existing plugins that used it. and because of this, felt like it'd be a waste of time re-writing the bookmarks/new-pages logic.
i'll definitely rework those two functions to use patchList though, if we think it'd be beneficial
There was a problem hiding this comment.
ah yeah, the trouble of moving people off updateList when it is likely fine for 99% of lists - is probably not worth it.
so yeah i'm good with leaving this as is
No description provided.