Page MenuHomePhabricator

[lib] Introduce MessageSpec.getLastUpdatedTime
ClosedPublic

Authored by ashoat on Tue, Nov 12, 12:41 PM.
Tags
None
Referenced Files
F3236898: D13913.id45760.diff
Wed, Nov 13, 1:32 AM
F3234780: D13913.id45767.diff
Tue, Nov 12, 10:23 PM
F3234779: D13913.id45760.diff
Tue, Nov 12, 10:23 PM
F3234748: D13913.diff
Tue, Nov 12, 10:22 PM
F3234727: D13913.id45767.diff
Tue, Nov 12, 10:18 PM
F3234675: D13913.id.diff
Tue, Nov 12, 10:00 PM
F3234533: D13913.id.diff
Tue, Nov 12, 9:15 PM
F3234495: D13913.diff
Tue, Nov 12, 9:05 PM
Subscribers
None

Details

Summary

This new method on the MessageSpec will allow us to address ENG-9558. If it returns null or undefined that means that the message should not bump the thread's timestamp. If it returns a Promise that means we don't yet know if it should bump the thread's timestamp.

For now, in this diff we're ignoring any messages whose MessageSpec.getLastUpdatedTime returns a Promise, and assuming it's the same as if it returned null. In later diffs we'll update the logic to be smarter.

Depends on D13912

Test Plan

Tested in combination with the rest of the stack. See video in the last diff

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Overall, this logic makes sense, but I think we should try to make it easier to understand.

lib/hooks/thread-time.js
41–77 ↗(On Diff #45767)

I have a hard time trying to understand this logic - I tried to read it a couple of times, but it still confuses me. Are there some ways of making it less confusing?
The things that make this logic confusing to me include:

  • Mutability of the variables (lastUpdatedTime and lastUpdatedAtLeastTime)
  • A couple of variables that are named in a similar way
  • A for loop that is broken based on an unintuitive condition (it is explained in the summary, though)

From these, mutability might be the main reason behind my feelings. I think we can consider adding some comments or avoiding mutating variables in a loop. There could be some other ways of making this more readable, though.

This revision is now accepted and ready to land.Wed, Nov 13, 4:11 AM
lib/hooks/thread-time.js
41–77 ↗(On Diff #45767)

Thanks for writing this out. Admittedly this logic is somewhat complex.

Regarding the pattern of using a let declaration to compose together multiple promises into one, we have this pattern in a couple other places in the codebase, such as here.

Given we're iterating through a loop to determine these values (which are later returned), I think it's okay to use let. The alternative would be to build a collection and evaluate it at the end, which would probably be a bit worse for performance and I'm not sure would be any more readable.

I'll add some comments to clarify the for loop and the mutable variables. Open to changing some variable names too, but to be honest, not sure what would be better.