Refactor Transaction and SyncEngine to use async/await #1983
Conversation
thebrianchen
force-pushed
the
bc/await-sync
branch
from
0956fa7 to
2f4f21e
Compare
There was a problem hiding this comment.
This mostly looks good and is nice clean up, but I think we perhaps undid a performance optimization (letting multiple promises execute in parallel) in one spot.
In general, I think this is 80% nice cleanup, but ~20% just code churn (the resulting code isn't particularly more readable than the prior code).
| ); | ||
| break; | ||
| } catch (error) { | ||
| if (isDocumentChangeMissingError(error)) { |
There was a problem hiding this comment.
Hrm. I think the prior code was a bit more precisely correct in that it was clear that the DocumentChangeMissingError would come from the getNewDocumentChanges() call, whereas now it could (theoretically) come from createSynthesizedRemoteEventForCurrentChange() or emitNewSnapsAndNotifyLocalStore().
It's probably fine, but I'm noticing that the promise chaining code is in some ways better... though the new code is perhaps more readable since the many line .then(..., ...) construct is probably easy to misread.
Anyway, this is probably good. I'm just thinking out loud.
There was a problem hiding this comment.
Yeah, I see what you mean. Would commenting the source of errors in the catch() block be helpful in this case?
There was a problem hiding this comment.
Probably not necessary, but wouldn't hurt (I leave it up to you. 😄 )
| ); | ||
| break; | ||
| } catch (error) { | ||
| if (isDocumentChangeMissingError(error)) { |
There was a problem hiding this comment.
Probably not necessary, but wouldn't hurt (I leave it up to you. 😄 )
No description provided.