If this function returns false, we will queue the operation for later. Some logic in the processing code can be simplified as the result of this change.
Depends on D12936
Details
Details
Tested this for one case - creating change_thread_settings update for a thread that isn't present in the store. Also tested that if the thread is in the store, the ops are being processed.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
I had previously suggested implemented a solution to this based on a tcomb validator. We'll need to specify IDs that have to be checked for collision again certain SQLite tables, and I was planning on using a tcomb validator approach for that. It seems like checking IDs for existence in certain SQLite tables is a very similar problem, and ought to be solved in a similar way.
Comment Actions
Not sure if @ashoat's comment demands changes to this diff, please make sure to address it
Comment Actions
I'm not sure. It sounds similar, but there are important differences:
- If isProcessingPossible === false we don't say anything about the validity of the operation. We only know that it can't be processed now.
- Some specs will have a more complicated logic, e.g. add-members-spec. I'm not sure if it will be possible to express a thing like this in a validator.
- There are multiple possible results of canBeProcessed function - currently the only valid reason is that a thread is missing, but maybe in some new specs there will be more. Regardless, we would need a validator to return something more than just a valid / invalid status. I think we're doing some more complicated transformation with validators, e.g. while checking for the new user ID, so it can be done.
It feels to me that validation and checking if an operation can be processed are two distinct things and using validators instead of canBeProcessed will be more complicated. But I'm open to giving it a try. If we decide to do that I would rather land this diff and modify the logic later, to make sure we unblock other things (in this case the whole queueing logic).
Comment Actions
Discussed this with @tomek in our 1:1 and we agreed to proceed with this approach for now.
I think that using validators for the most common cases makes sense, but I agree there are exceptions where we'll need to do something more complicated. For those complicated cases, I think having this canBeProcessed function on the spec makes sense
| lib/types/dm-ops.js | ||
|---|---|---|
| 288–291 ↗ | (On Diff #42973) | When I introduced this, it was intentionally not read-only. The idea is that the caller can do anything they want with the result If there is a strong reason for making this read-only now, I would think we'd want $ReadOnlyArray instead of Array But in that case, it's just ProcessDMOpsPayload, which is defined below Can we revert this change? |
| lib/types/dm-ops.js | ||
|---|---|---|
| 288–291 ↗ | (On Diff #42973) |
I think that can be true for a lot of our types, except e.g. things returned from Redux and props. I'm not sure why we want to have a different approach here.
Sure, I'll do that! |
Comment Actions
I think that can be true for a lot of our types, except e.g. things returned from Redux and props. I'm not sure why we want to have a different approach here.
I don't agree that this is a "different approach". I try to consistently use this approach, except in cases where the returned value really should not be modified.