-
Notifications
You must be signed in to change notification settings - Fork 14
Refactor activity log types #127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e65c7f9
540558d
a1d7eed
7d4a535
5f1b0ac
faeea7d
c0c7c84
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,47 +1,46 @@ | ||||||
'use strict' | ||||||
|
||||||
/** @typedef {import('./typings').ActivityEvent} ActivityEvent */ | ||||||
/** @typedef {import('./typings').ActivityEntry} ActivityEntry */ | ||||||
/** @typedef {import('./typings').Activity} Activity */ | ||||||
/** @typedef {import('./typings').RecordActivityOptions} RecordActivityOptions */ | ||||||
|
||||||
const Store = require('electron-store') | ||||||
const crypto = require('node:crypto') | ||||||
|
||||||
const activityLogStore = new Store({ | ||||||
name: 'activity-log' | ||||||
}) | ||||||
|
||||||
class ActivityLog { | ||||||
#entries | ||||||
#lastId | ||||||
|
||||||
constructor () { | ||||||
this.#entries = loadStoredEntries() | ||||||
this.#lastId = Number(this.#entries.at(-1)?.id ?? 0) | ||||||
} | ||||||
|
||||||
/** | ||||||
* @param {ActivityEvent} args | ||||||
* @returns {ActivityEntry} | ||||||
* @param {RecordActivityOptions} args | ||||||
* @returns {Activity} | ||||||
*/ | ||||||
recordEvent ({ source, type, message }) { | ||||||
const nextId = ++this.#lastId | ||||||
/** @type {ActivityEntry} */ | ||||||
const entry = { | ||||||
id: String(nextId), | ||||||
recordActivity ({ source, type, message }) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is a method of
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good idea! |
||||||
/** @type {Activity} */ | ||||||
const activity = { | ||||||
id: crypto.randomUUID(), | ||||||
timestamp: Date.now(), | ||||||
source, | ||||||
type, | ||||||
message | ||||||
} | ||||||
// Freeze the data to prevent ActivityLog users from accidentally changing our store | ||||||
Object.freeze(entry) | ||||||
Object.freeze(activity) | ||||||
|
||||||
this.#entries.push(entry) | ||||||
this.#entries.push(activity) | ||||||
|
||||||
if (this.#entries.length > 100) { | ||||||
// Delete the oldest entry to keep ActivityLog at constant size | ||||||
// Delete the oldest activity to keep ActivityLog at constant size | ||||||
this.#entries.shift() | ||||||
} | ||||||
this.#save() | ||||||
return entry | ||||||
return activity | ||||||
} | ||||||
|
||||||
getAllEntries () { | ||||||
|
@@ -56,12 +55,12 @@ class ActivityLog { | |||||
} | ||||||
|
||||||
#save () { | ||||||
activityLogStore.set('events', this.#entries) | ||||||
activityLogStore.set('activities', this.#entries) | ||||||
} | ||||||
} | ||||||
|
||||||
/** | ||||||
* @returns {ActivityEntry[]} | ||||||
* @returns {Activity[]} | ||||||
*/ | ||||||
function loadStoredEntries () { | ||||||
// A workaround to fix false TypeScript errors | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,22 @@ | ||
export type ActivitySource = 'Station' | 'Saturn'; | ||
export type ActivityEventType = 'info' | 'error'; | ||
export type ActivityType = 'info' | 'error'; | ||
|
||
export interface ActivityEvent { | ||
type: ActivityEventType; | ||
export interface Activity { | ||
id: string; | ||
timestamp: number; | ||
type: ActivityType; | ||
source: ActivitySource; | ||
message: string; | ||
} | ||
|
||
export interface ActivityEntry extends ActivityEvent { | ||
id: string; | ||
timestamp: number; | ||
export interface RecordActivityOptions { | ||
type: ActivityType; | ||
source: ActivitySource; | ||
message: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I removed the inheritance, because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It makes sense to remove inheritance 👍🏻 An alternative approach that does not duplicate property definitions: export type RecordActivityOptions = Pick<Activity, 'type' | 'source' | 'message'>; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cool I like that! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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'>; |
||
} | ||
|
||
export interface Context { | ||
recordActivity(event: ActivityEvent): void; | ||
recordActivity(activity: RecordActivityOptions): void; | ||
resumeActivityStream(): void; | ||
|
||
showUI: () => void | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ import ReactDOM from 'react-dom/client' | |
import { BrowserRouter } from 'react-router-dom' | ||
import App from './App' | ||
import './index.css' | ||
import { Activity } from '../../main/typings' | ||
|
||
ReactDOM.createRoot( | ||
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion | ||
|
@@ -15,8 +16,22 @@ ReactDOM.createRoot( | |
</React.StrictMode> | ||
) | ||
|
||
window.electron.onActivityLogged(entry => { | ||
console.log('[ACTIVITY] %j', entry) | ||
const activities: Activity[] = [] | ||
|
||
window.electron.onActivityLogged(activity => { | ||
activities.push(activity) | ||
// In case two activities were recorded in the same millisecond, fall back to | ||
// sorting by their IDs, which are guaranteed to be unique and therefore | ||
// provide a stable sorting. | ||
activities.sort((a, b) => { | ||
return a.timestamp !== b.timestamp | ||
? b.timestamp - a.timestamp | ||
: a.id.localeCompare(b.id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure ids created by 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. alternative idea: Is there a timing api with such precision that it's guaranteed that two successive calls won't get the same values? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
if (activities.length > 100) activities.shift() | ||
|
||
console.log('[ACTIVITY] %j', activity) | ||
console.log('[ACTIVITIES]', activities) | ||
}) | ||
|
||
window.electron.resumeActivityStream().then(() => { | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 juggleActivity
,Event
andEntry
in your mind, and the variable names are nicely consistent.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also rename this to use
Event
instead ofActivity
, which is the more common term, but I do think hereActivity
works better, because it's clearer that we're referring to a product concept, and not a generic event.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 likeRecordActivityFields
orNewActivityProps
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use
NewActivityArgs
then.