Refactor activity log types #127

juliangruber · 2022-09-14T15:29:50Z

To help clarify my reasoning in #110 (comment), I decided to code it up.

@bajtos what do you think?

juliangruber · 2022-09-14T16:10:35Z

The types are incomplete, will fix

juliangruber · 2022-09-14T16:19:34Z

main/activity-log.js

@@ -1,31 +1,30 @@
 'use strict'

 /** @typedef {import('./typings').ActivityEvent}  ActivityEvent */
-/** @typedef {import('./typings').ActivityEntry}  ActivityEntry */
+/** @typedef {import('./typings').RecordActivityOptions}  RecordActivityOptions */


I think we can simplify the types by thinking about them this way: One is the actual activity (Activity), the other is the options passed to .recordActivity (RecordActivityOptions). To me this is clearer than having to juggle Activity, Event and Entry in your mind, and the variable names are nicely consistent.

We could also rename this to use Event instead of Activity, which is the more common term, but I do think here Activity works better, because it's clearer that we're referring to a product concept, and not a generic event.

I like the idea to user Activity to describe the data structure we are storing in our log 👍🏻

I have mixed feelings about the name RecordActivityOptions. In my mind, options is for optional configuration tweaking the behaviour of a function. How about something like RecordActivityFields or NewActivityProps?

Sounds good, I tried to look up prior art on this and found that the types for node for example call arguments options but their type ...Args:

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/3b2cb0f873d4caca3e4fc01aeaf9f8ec40852d8f/types/node/http.d.ts#L138-L139

Either would work for me 👍

Let's use NewActivityArgs then.

juliangruber · 2022-09-14T16:28:11Z

main/typings.d.ts

+export interface RecordActivityOptions {
+  type: ActivityType;
+  source: ActivitySource;
+  message: string;


I removed the inheritance, because Activity is the main type, and RecordActivityOptions more a partial of it.

It makes sense to remove inheritance 👍🏻

An alternative approach that does not duplicate property definitions:

export type RecordActivityOptions = Pick<Activity, 'type' | 'source' | 'message'>;

cool I like that!

On the second thought, this is probably more future proof when it comes to adding new Activity props:

export type RecordActivityOptions = Omit<Activity, 'id' | 'timestamp'>;

renderer/src/main.tsx

juliangruber · 2022-09-15T07:13:08Z

renderer/src/main.tsx

+    return a.timestamp !== b.timestamp
+      ? b.timestamp - a.timestamp
+      : a.id.localeCompare(b.id)
+  })


if we moved sorting to the backend, then we wouldn't have to sort on every new event. Otherwise I think we could do it in either location, and having it on the view layer might be easier to get right wrt race conditions

juliangruber · 2022-09-15T07:14:34Z

Error: main/test/activity-log.test.js(55,5): error TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
Error: Process completed with exit code 2.

@bajtos I don't understand why this isn't showing up in main ci 🤔

bajtos · 2022-09-15T08:17:24Z

main/activity-log.js

-    /** @type {ActivityEntry} */
-    const entry = {
-      id: String(nextId),
+  recordActivity ({ source, type, message }) {


Since this is a method of ActivityLog class, maybe we can shorten the name to simply record?

Suggested change

recordActivity ({ source, type, message }) {

record ({ source, type, message }) {

bajtos · 2022-09-15T08:18:53Z

main/typings.d.ts

+export interface RecordActivityOptions {
+  type: ActivityType;
+  source: ActivitySource;
+  message: string;


It makes sense to remove inheritance 👍🏻

An alternative approach that does not duplicate property definitions:

export type RecordActivityOptions = Pick<Activity, 'type' | 'source' | 'message'>;

bajtos · 2022-09-15T08:20:33Z

renderer/src/main.tsx

+  activities.sort((a, b) => {
+    return a.timestamp !== b.timestamp
+      ? b.timestamp - a.timestamp
+      : a.id.localeCompare(b.id)


Are you sure ids created by crypto.randomUUID() monotonically increasing over time?

Maybe the order of events reported in the same millisecond does not matter and that's why we can fallback our sorting comparisons to comparing ids?

I think it would be great to add a code comment to clarify this.

They don't increase, and that's not an issue in my mind. As long as we sort deterministically, no human will have noticed that the events from the same millisecond have been mixed up. And we sort deterministically by giving events IDs when storing them. I'll add a comment 👍

alternative idea: Is there a timing api with such precision that it's guaranteed that two successive calls won't get the same values?

process.hrtime() seems to work:

> [process.hrtime(), process.hrtime()] [ [ 845635, 876114697 ], [ 845635, 876117208 ] ]

As long as we sort deterministically, no human will have noticed that the events from the same millisecond have been mixed up.

Unless these two events happen to be something like "Starting the station" and "Starting Saturn module". It would look weird to have them in the reverse direction.

Anyhow, I think this does not really matter, especially not now.

bajtos · 2022-09-15T11:18:42Z

Let's merge this into my branch and then work together there.

prototype activity log layout

e65c7f9

juliangruber requested a review from bajtos September 14, 2022 15:29

juliangruber mentioned this pull request Sep 14, 2022

Activity Log backend and WebUI #110

Merged

juliangruber marked this pull request as draft September 14, 2022 16:10

fix types

540558d

juliangruber marked this pull request as ready for review September 14, 2022 16:17

juliangruber added 3 commits September 14, 2022 18:24

refactor types

a1d7eed

refactor names

7d4a535

refactor variable names

5f1b0ac

juliangruber changed the title ~~prototype activity log layout~~ prototype activity log types Sep 14, 2022

juliangruber commented Sep 14, 2022

View reviewed changes

juliangruber changed the title ~~prototype activity log types~~ Refactor activity log types Sep 14, 2022

juliangruber commented Sep 15, 2022

View reviewed changes

renderer/src/main.tsx Outdated Show resolved Hide resolved

fix sorting bug

faeea7d

juliangruber commented Sep 15, 2022

View reviewed changes

bajtos reviewed Sep 15, 2022

View reviewed changes

docs

c0c7c84

bajtos merged commit d8e010f into feat-activity-log-backend Sep 15, 2022

bajtos deleted the prototype-activity-log-julian branch September 15, 2022 11:18

bajtos mentioned this pull request May 3, 2023

Integrate Station Core #621

Merged

	recordActivity ({ source, type, message }) {
	record ({ source, type, message }) {

Refactor activity log types #127

Refactor activity log types #127

Uh oh!

Conversation

juliangruber commented Sep 14, 2022

Uh oh!

juliangruber commented Sep 14, 2022

Uh oh!

juliangruber Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliangruber commented Sep 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos commented Sep 15, 2022

Uh oh!

Uh oh!

juliangruber Sep 14, 2022 •

edited

Loading