fix: Correctly persist createdAt attribute #119
Conversation
fixes issue #118 Signed-off-by: Jakub Koci <[email protected]>
| @@ -17,7 +18,7 @@ export class BasicMessageRecord extends BaseRecord implements BasicMessageStorag | |||
| public readonly type = BasicMessageRecord.type; | |||
|
|
|||
| public constructor(props: BasicMessageStorageProps) { | |||
| super(props.id || uuid()); | |||
| super(props.id || uuid(), props.createdAt || Date.now()); | |||
There was a problem hiding this comment.
TypeScript 4.0 introduced some new short circuit assignment operators. Which are perfect for this usage.
| super(props.id || uuid(), props.createdAt || Date.now()); | |
| super(props.id ?? uuid(), props.createdAt ?? Date.now()); |
There was a problem hiding this comment.
Ah, I see. That's nice. I can't even count how often I solve issues with if (number) ... 😄
There was a problem hiding this comment.
Hmm, as I'm thinking about it. Are they really perfect for this usage? Maybe values '' and 0 are not quite valid as id or createAt
There was a problem hiding this comment.
But that's perhaps the responsibility of some eventual validation. Ok, I'll change it to ??
There was a problem hiding this comment.
Well yeah, 0 is probably not a commonly used value. However it is unexpected behaviour IMO to change the value without notic. When I pass a value of 0 and it is automatically changed to Date.now without warning or error that is weird I think. Same with empty string.
src/lib/storage/ConnectionRecord.ts
Outdated
| @@ -49,7 +51,7 @@ export class ConnectionRecord extends BaseRecord implements ConnectionStoragePro | |||
| public readonly type = ConnectionRecord.type; | |||
|
|
|||
| public constructor(props: ConnectionStorageProps) { | |||
| super(props.id); | |||
| super(props.id || uuid(), props.createdAt || Date.now()); | |||
src/lib/storage/CredentialRecord.ts
Outdated
| @@ -27,7 +28,7 @@ export class CredentialRecord extends BaseRecord implements CredentialStoragePro | |||
| public state: CredentialState; | |||
|
|
|||
| public constructor(props: CredentialStorageProps) { | |||
| super(props.id ? props.id : uuid()); | |||
| super(props.id || uuid(), props.createdAt || Date.now()); | |||
| @@ -18,20 +19,20 @@ class TestRecord extends BaseRecord { | |||
| public readonly type = TestRecord.type; | |||
|
|
|||
| public constructor(props: TestRecordProps) { | |||
| super(props.id || uuid()); | |||
| super(props.id || uuid(), props.createdAt || Date.now()); | |||
| @@ -15,7 +17,7 @@ export class ProvisioningRecord extends BaseRecord { | |||
| public readonly type = ProvisioningRecord.type; | |||
|
|
|||
| public constructor(props: ProvisioningRecordProps) { | |||
| super(props.id); | |||
| super(props.id || uuid(), props.createdAt || Date.now()); | |||
Hmm, that interesting, but how does it actually work? I'm looking around but can't find anything about that 😄 |
I linked to the wrong thing. What I actually mean is "nullish coalescing" Basically when you use Also see: https://github.com/tc39/proposal-nullish-coalescing/ |
Signed-off-by: Jakub Koci <[email protected]>
I needed to move
createAtattribute initialization up in the inheritance chain to every particular record. I'm not sure if this is a good approach in the long-term but it at least fixes the issue for now. I also alignedidattribute initialization across all records, so even if this is a wrong approach we have it wrong in the same way everywhere :)