feat: lastModified and db for diagram list COMPASS-9398 #6951
Conversation
paula-stacho
added
the
feature flagged
| export function useFormattedDate(timestamp: number): string; | ||
| export function useFormattedDate(timestamp?: number): string | undefined; | ||
| export function useFormattedDate(timestamp?: number): string | undefined { |
There was a problem hiding this comment.
What you were running into here is a difference between overload signatures and overload implementation and is indeed a "common source of confusion" even mentioned as such in the TS documentation 🙂 The last function here, the overload implementation, is not "visible" to the outside
There was a problem hiding this comment.
oooh so I learned two TS lessons today
| @@ -48,12 +78,16 @@ export function DiagramCard({ | |||
| onRename, | |||
| onDelete, | |||
| }: { | |||
| diagram: MongoDBDataModelDescription; | |||
| diagram: MongoDBDataModelDescription & { | |||
| lastModified?: number; | |||
There was a problem hiding this comment.
How is this value ever optional? Shouldn't the initial save count as "modified" too?
There was a problem hiding this comment.
technically yeah the array should never be empty and so the derived value should always be there. I checked and looks like z.nonempty() could work here
There was a problem hiding this comment.
okay, typed the edits so that they are non empty. forcing us to ensure this in the reducer as well, which is good.
also good learning for me, I didn't expect that TS would understand this.
| ); | ||
| return { | ||
| ...item, | ||
| lastModified: Date.parse(item.edits[item.edits.length - 1].timestamp), |
There was a problem hiding this comment.
I think when we usually refer to last modified we specifically mean when the file was saved by the storage, wouldn't it be easier to just write this value as part of the store logic instead of recalculating it here all the time?
There was a problem hiding this comment.
Yeah might be. Following the tech design here. We could achieve the same if we always save the last edit's timestamp also on the top level. But I'm actually not sure if lastModified should work like this with the undo. Shouldn't undo also count as modification? In that case we definitely need to store this. cc @Anemy - sorry that I didn't notice this when reviewing the design
There was a problem hiding this comment.
You also have a case of changing the name which counts as modification I think. Applying undo might also get confusing if we just use the timestamps (like imaging undoing to something that was timestamped a long time ago and the item that you might expect at the top is not at the bottom of the list). I really don't think we want to bind stored file modification action to edits themselves
Description
Note: I made the cards a bit smaller than the designs, they looked too empty with only the db.
Overloads confusion
Hooks can't be called conditionally, so I made the hook useFormattedDate hook accept optional value. In that case the return type is of course optional too. This is represented with function overload. Unfortunately, the overloaded type is getting lost somewhere in the exports/imports, so in the end there is only one function signature.
Update:
I figured out what works - to repeat the overload signature. Looks like the last one is not transferred. I am still confused.
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes