feat: Use type discriminators for event payload. #89

ffMathy · 2023-03-22T10:13:39Z

I have added automatic tests where applicable
The PR title is suitable as a release note
The PR contains a description of what has been changed
The description contains manual test instructions

Changes

Currently, there are two main issues with the typings of Ftrack events.

The EventData type is sometimes incorrect. It assumes that the data for the event is always for an ftrack.update event. So for most other events, the type is incorrect, which is a bug in my opinion.
It doesn't support type discriminators to intelligently determine the type. You end up with quite a lot of type casts etc.

Here's an example that explains the issue before my pull request.

eventHub.subscribe('topic=ftrack.update', event => {
   //here, before this pull request, event.data is of type EventData which is correct for the ftrack.update event
});

eventHub.subscribe('topic=ftrack.action.discover', event => {
   //here, before this pull request, event.data is still of type EventData which is incorrect for the ftrack.action.discover event
});

After my pull request, the TypeScript compiler understands the discriminator and can narrow the event down without casting.

eventHub.subscribe("some-query", event => {
  if(event.topic === "ftrack.action.discover") {
    //event.data is now of type ActionDiscoverEventPayload automatically!
    const firstSelection = event.data.selection[0]; //this doesn't give compile errors!
  } else if(event.topic === "ftrack.update") {
    //event.data is now of type UpdateEventPayload automatically!
    const firstEntity = event.data.entities[0]; //this doesn't give compile errors!
  } else {
    //here, event.data is now of type unknown.
  }
})

Test

It should be tested by running tests and compiling the code. This has already been done.

gismya

A quick check now, will need to compare with the code and the documentation to make a deeper review.

source/event_hub.ts

ffMathy · 2023-03-22T14:54:12Z

Thanks! Much appreciated. I fixed the comments you had now so far. Let me know when more arrives ❤️

ffMathy · 2023-03-22T14:56:02Z

By the way, here's TypeScript's official Discriminate Types example if you want to try out how something like this would work:
https://www.typescriptlang.org/play#example/discriminate-types

More documentation here: https://basarat.gitbook.io/typescript/type-system/discriminated-unions

gismya · 2023-03-23T08:26:49Z

I appreciate the teaching mindset you are exhibiting. But I understand Discriminate Types, that's no issue. I will try to make time to review your changes today or tomorrow.

ffMathy · 2023-03-23T08:49:11Z

Yeah sorry, didn't mean to be condescending in any way 🙏

When I saw the private methods that weren't really private (but just prefixed with underscore like you'd do in Python), I was just unsure if the team maintaining this repo had experience with the more advanced TypeScript features, if they came from a Python background ❤️

I see now that you do! 👌

gismya · 2023-03-24T13:20:46Z

The issue was a bit thornier than I thought and will have to get some more input from other people on how this actually works next week. Found a few things that should be changed until then.

Suggested edit:

diff --git a/source/event_hub.ts b/source/event_hub.ts
index f784715..3154ed9 100644
--- a/source/event_hub.ts
+++ b/source/event_hub.ts
@@ -20,9 +20,10 @@ interface BaseActionData {
 
 interface BaseEventPayload {
   target: string;
-  inReplyToEvent: string;
+  inReplyToEvent?: string;
   source: EventSource;
   id: string;
+  sent?: boolean;
 }
 
 export interface ActionDiscoverEventPayload extends BaseEventPayload {
@@ -32,8 +33,9 @@ export interface ActionDiscoverEventPayload extends BaseEventPayload {
 
 export interface ActionLaunchEventData extends BaseActionData {
   actionIdentifier: string;
-  description: string;
-  label: string;
+  description?: string;
+  label?: string;
+  applicationIdentifier?: string;
 }
 
 export interface ActionLaunchEventPayload extends BaseEventPayload {

ffMathy · 2023-03-25T10:54:54Z

The given changes have been applied! Thank you for looking into all of this so fast and with such high priority. It really sparks a motivation to make many more pull requests in the future.

gismya · 2023-03-29T14:36:15Z

The more I look into this the less rigid the event structure seems. There are always a bunch of exceptions to the general rules. To ensure nothing breaks at runtime it seems best to assume everything can come back as undefined and make sure to handle that. We might have to make even more things optional (And there's a handful I might actually be able to put back as not optional).

After these changes, I'll approve the PR and set up a task to make an even more thorough review of the entire event hub. The changes will not make everything 100% correct for all event topics yet, but it's definitely closer than the current state.

Suggested edit:

diff --git a/source/event_hub.ts b/source/event_hub.ts
index dc7d820..5adba5f 100644
--- a/source/event_hub.ts
+++ b/source/event_hub.ts
@@ -44,14 +44,14 @@ export interface ActionLaunchEventPayload extends BaseEventPayload {
 }
 
 export interface UpdateEventData {
-  entities: EventEntity[];
-  pushToken: string;
-  parents: string[];
-  user: {
+  entities?: EventEntity[];
+  pushToken?: string;
+  parents?: string[];
+  user?: {
     userid: string;
     name: string;
   };
-  clientToken: string;
+  clientToken?: string;
 }
 
 export interface UpdateEventPayload extends BaseEventPayload {
@@ -81,20 +81,20 @@ export interface EventSource {
 }
 
 export interface EventEntity {
-  entity_type: string;
-  keys: string[];
-  objectTypeId: string;
-  entityType: string;
-  parents: {
+  entity_type?: string;
+  keys?: string[];
+  objectTypeId?: string;
+  entityType?: string;
+  parents?: {
     entityId: string;
     entityType: string;
     entity_type: string;
     parentId?: string;
   }[];
-  parentId: string;
-  action: string;
-  entityId: string;
-  changes: Data;
+  parentId?: string;
+  action?: string;
+  entityId?: string;
+  changes?: Data;
 }
 
 export interface SubscriberMetadata {

ffMathy · 2023-03-29T17:50:54Z

Do those exceptions have distinguishable scenarios that can be handled by discriminators?

Or do some of them always go in pairs? For instance, (hypothetically) if parentId is set, is entityId then also always set, etc. If that's the case, we can still have separate types for them, and then have unions without type discriminators.

Then it's still more flexible and stable.

I agree that it doesn't have to be perfect. Just better every time. That being said, we could also try to make some of them required (for the ones where we aren't sure) and turn them back to optional if it becomes a problem. I'd actually rather do that than the other way around. But it's of course up to you!

Thank you so much for the effort so far - this is amazing stuff.

ffMathy · 2023-03-29T17:53:17Z

I have applied the changes now. Once again thanks!

As for when you'll get back to making the types better (and look at the event hub), I think splitting all the different types into separate files could also do wonders to the clarity (but I assume you know this already - just sharing my opinion in case).

gismya · 2023-03-30T07:24:42Z

Suggested edit:

diff --git a/source/event_hub.ts b/source/event_hub.ts
index 5adba5f..a4369e4 100644
--- a/source/event_hub.ts
+++ b/source/event_hub.ts
@@ -578,7 +578,9 @@ export class EventHub {
    */
   _handleReply(eventPayload: EventPayload) {
     this.logger.debug("Reply received", eventPayload);
-    const onReplyCallback = this._replyCallbacks[eventPayload.inReplyToEvent];
+    const onReplyCallback = !eventPayload.inReplyToEvent
+      ? null
+      : this._replyCallbacks[eventPayload.inReplyToEvent];
     if (onReplyCallback) {
       onReplyCallback(eventPayload);
     }

gismya · 2023-03-30T07:30:39Z

I'm of the viewpoint that it's better to have things that possibly might be missing to be optional. It can be a hassle writing null-checks, but a runtime crash because data that "should" be there isn't is worse. With that said, I'll investigate if there are more things we can ensure are there.

Had a small suggestion that needs to be fixed, but then I'm happy. I'll assign a colleague to do a final sanity check as well.

ffMathy · 2023-03-30T13:28:52Z

Fair enough, that is understandable!

I applied the most recent fixes now! 🙏

gismya

Ping @jimmycallin for a final sanity check

ffMathy added 3 commits March 22, 2023 10:47

Made event payloads discriminatable.

5b535b6

Fix typo.

0bcac95

Prettier.

3497047

ffMathy requested a review from a team as a code owner March 22, 2023 10:13

ffMathy added 2 commits March 22, 2023 11:16

Merge branch 'main' into patch-2

4a8c7de

Minor change to typings.

3d05f29

gismya reviewed Mar 22, 2023

View reviewed changes

source/event_hub.ts Show resolved Hide resolved

source/event_hub.ts Outdated Show resolved Hide resolved

Review fixes.

aa7f2bd

ffMathy added 2 commits March 25, 2023 01:13

Merge branch 'main' into patch-2

2bcbe39

Review fixes.

ae2a027

Update event_hub.ts

df06f33

Update event_hub.ts

1781833

gismya approved these changes Mar 30, 2023

View reviewed changes

gismya requested a review from jimmycallin March 30, 2023 13:50

jimmycallin approved these changes Mar 31, 2023

View reviewed changes

gismya merged commit 54a3462 into ftrackhq:main Mar 31, 2023

gismya pushed a commit to gismya/ftrack-javascript that referenced this pull request Apr 11, 2023

feat: Use type discriminators for event payload. (ftrackhq#89)

2e5b8da

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: Use type discriminators for event payload. #89

feat: Use type discriminators for event payload. #89

Uh oh!

ffMathy commented Mar 22, 2023 •

edited

Loading

Uh oh!

gismya left a comment

Uh oh!

Uh oh!

Uh oh!

ffMathy commented Mar 22, 2023

Uh oh!

ffMathy commented Mar 22, 2023 •

edited

Loading

Uh oh!

gismya commented Mar 23, 2023

Uh oh!

ffMathy commented Mar 23, 2023

Uh oh!

gismya commented Mar 24, 2023 •

edited

Loading

Uh oh!

ffMathy commented Mar 25, 2023

Uh oh!

gismya commented Mar 29, 2023 •

edited

Loading

Uh oh!

ffMathy commented Mar 29, 2023

Uh oh!

ffMathy commented Mar 29, 2023

Uh oh!

gismya commented Mar 30, 2023

Uh oh!

gismya commented Mar 30, 2023

Uh oh!

ffMathy commented Mar 30, 2023

Uh oh!

gismya left a comment

Uh oh!

Uh oh!

feat: Use type discriminators for event payload. #89

feat: Use type discriminators for event payload. #89

Uh oh!

Conversation

ffMathy commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Test

Uh oh!

gismya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ffMathy commented Mar 22, 2023

Uh oh!

ffMathy commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gismya commented Mar 23, 2023

Uh oh!

ffMathy commented Mar 23, 2023

Uh oh!

gismya commented Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ffMathy commented Mar 25, 2023

Uh oh!

gismya commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ffMathy commented Mar 29, 2023

Uh oh!

ffMathy commented Mar 29, 2023

Uh oh!

gismya commented Mar 30, 2023

Uh oh!

gismya commented Mar 30, 2023

Uh oh!

ffMathy commented Mar 30, 2023

Uh oh!

gismya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ffMathy commented Mar 22, 2023 •

edited

Loading

ffMathy commented Mar 22, 2023 •

edited

Loading

gismya commented Mar 24, 2023 •

edited

Loading

gismya commented Mar 29, 2023 •

edited

Loading