feat(ui-collection): add a proto optimistic() method to collections #183
Conversation
Fen747
force-pushed
the
feat/optimistic-updates
branch
from
1712f7b
to
b28f7cc
Compare
| */ | ||
| public optimistic(_id: ObjectId | string, document: T) { | ||
| const stringId = _id instanceof ObjectId ? _id.toString() : _id; | ||
| const operationName = ""; // ! how do we determine this here ? |
There was a problem hiding this comment.
based on this.getName(), we'll need to do it for Find FindOne and Count maybe? Or give some opts?
There was a problem hiding this comment.
The idea would be to have this behavior by default.
A developper will prefer to write :
collection.updateOne(_id, document)and expect the cache to be automatically update (or disable that behavior with a optimisticReponse: false option) than to have to manually add an additional for each update in his software :
collection.updateOne(_id,
document,
{ optimisticResponse: collection.optimistic(_id, document) }
)We could add an additional optional argument with the queryName, that would default to [Collection]UpdateOne
There was a problem hiding this comment.
Proposition commited : 2a6aac5
| collectionName[collectionName.length - 1] === "s" | ||
| ? collectionName.slice(0, -1) | ||
| : collectionName, // we could use https://www.npmjs.com/package/pluralize | ||
| ...document, // should we deepclone here just in case ? |
There was a problem hiding this comment.
I think you need to transform the document into a graph-like model of it.
There was a problem hiding this comment.
I'm not sure of hat you mean.
What we need here is to provide the exact same values as in the input : https://www.apollographql.com/docs/react/performance/optimistic-ui/
Fen747
force-pushed
the
feat/optimistic-updates
branch
3 times, most recently
from
d4f7ff2
to
4a5d139
Compare
| [operationName]: { | ||
| _id: stringId, | ||
| __typename: | ||
| collectionName[collectionName.length - 1] === "s" |
There was a problem hiding this comment.
This is not a reliable way to do this, we might need to add a getTypeName() { }
There was a problem hiding this comment.
Moved it to collection.getTypeName, keeping this dumb implementation. Probably that the best would be to have the generator to auto implement an override of getTypename and jut keep this dumb implementation "just in case" ?
Fen747
force-pushed
the
feat/optimistic-updates
branch
from
35d5888
to
469e726
Compare
| @@ -360,6 +379,12 @@ export abstract class Collection<T = null> { | |||
| return this.hybridFind(false, query, body); | |||
| } | |||
|
|
|||
| protected optimisticUpdates = true; | |||
|
|
|||
| public enableOptimisticUpdates(option: boolean) { | |||
There was a problem hiding this comment.
I don't know if this is a good approach, at what step would you actually call this enableOptimisticUpdates? In every component? Rather have something: isOptimisticUpdateEnabled() { return true }
| // `${this.getName()}UpdateOne` | ||
| [operationName]: { | ||
| _id: stringId, | ||
| __typename: this.getTypeName(), |
There was a problem hiding this comment.
Yep, we should also add this to blueprint collection generation to return the entity name so we don't have situations like "Goose" "Geese"
TODO:
Related: