Skip to content
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

Use TS Morph to generate type predicates #1387

Closed
wants to merge 2 commits into from

Conversation

Roaders
Copy link
Contributor

@Roaders Roaders commented Oct 15, 2024

adresses #1268

(apologies for the second PR on this. I had stupidly created my last PR #1278 from a silly branch and then overwrote it).

This is a very rough cut and really just proves that what I was planning does work.

We could generate the predicates in a different file if we wanted to. We could also generate union types of the request and response messages if we wanted to.

I had some issues getting ts-node working initially hence the extra tsconfig file. We can move the generate-type-predicates file into the root of the project if desired.


THIS SOFTWARE IS CONTRIBUTED SUBJECT TO THE TERMS OF THE FINOS CORPORATE CONTRIBUTOR LICENSE AGREEMENT.

THIS SOFTWARE IS LICENSED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE AND ANY WARRANTY OF NON-INFRINGEMENT, ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. THIS SOFTWARE MAY BE REDISTRIBUTED TO OTHERS ONLY BY EFFECTIVELY USING THIS OR ANOTHER EQUIVALENT DISCLAIMER IN ADDITION TO ANY OTHER REQUIRED LICENSE TERMS.

@Roaders Roaders requested a review from a team as a code owner October 15, 2024 11:35
Copy link

netlify bot commented Oct 15, 2024

Deploy Preview for fdc3 ready!

Name Link
🔨 Latest commit 6cf70dc
🔍 Latest deploy log https://app.netlify.com/sites/fdc3/deploys/670e53a0621dc8000859bb01
😎 Deploy Preview https://deploy-preview-1387--fdc3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Roaders Roaders changed the base branch from main to fdc3-for-web October 15, 2024 11:37
}
}

export type RequestMessage = AddContextListenerRequest | AddEventListenerRequest | AddIntentListenerRequest | BroadcastRequest | ContextListenerUnsubscribeRequest | CreatePrivateChannelRequest | EventListenerUnsubscribeRequest | FindInstancesRequest | FindIntentRequest | FindIntentsByContextRequest | GetAppMetadataRequest | GetCurrentChannelRequest | GetCurrentContextRequest | GetInfoRequest | GetOrCreateChannelRequest | GetUserChannelsRequest | HeartbeatAcknowledgementRequest | IntentListenerUnsubscribeRequest | IntentResultRequest | JoinUserChannelRequest | LeaveCurrentChannelRequest | OpenRequest | PrivateChannelAddEventListenerRequest | PrivateChannelDisconnectRequest | PrivateChannelUnsubscribeEventListenerRequest | RaiseIntentForContextRequest | RaiseIntentRequest;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These names RequestMessage ResponseMessage and EventMessage probably need to change. I am open to suggestions!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing this, there is another way. We already generate 3 types that will only validate against these groupings:

  • AppRequestMessage,
  • AgentResponseMessage,
  • AgentEventMessage

and each has a union of the type field string values that apply:

  • RequestMessageType,
  • ResponseMessageType,
  • EventMessageType

E.g.

export interface AppRequestMessage {
/**
* Metadata for a request message sent by an FDC3-enabled app to a Desktop Agent.
*/
meta: AppRequestMessageMeta;
/**
* The message payload typically contains the arguments to FDC3 API functions.
*/
payload: { [key: string]: any };
/**
* Identifies the type of the message and it is typically set to the FDC3 function name that
* the message relates to, e.g. 'findIntent', with 'Request' appended.
*/
type: RequestMessageType;
}
/**
* Metadata for a request message sent by an FDC3-enabled app to a Desktop Agent.
*/
export interface AppRequestMessageMeta {
requestUuid: string;
/**
* Field that represents the source application that a request or response was received
* from. Please note that this may be set by an app or Desktop Agent proxy for debugging
* purposes but a Desktop Agent should make its own determination of the source of a message
* to avoid spoofing.
*/
source?: AppIdentifier;
timestamp: Date;
}
/**
* Identifies the type of the message and it is typically set to the FDC3 function name that
* the message relates to, e.g. 'findIntent', with 'Request' appended.
*/
export type RequestMessageType = "addContextListenerRequest" | "addEventListenerRequest" | "addIntentListenerRequest" | "broadcastRequest" | "contextListenerUnsubscribeRequest" | "createPrivateChannelRequest" | "eventListenerUnsubscribeRequest" | "findInstancesRequest" | "findIntentRequest" | "findIntentsByContextRequest" | "getAppMetadataRequest" | "getCurrentChannelRequest" | "getCurrentContextRequest" | "getInfoRequest" | "getOrCreateChannelRequest" | "getUserChannelsRequest" | "heartbeatAcknowledgementRequest" | "intentListenerUnsubscribeRequest" | "intentResultRequest" | "joinUserChannelRequest" | "leaveCurrentChannelRequest" | "openRequest" | "privateChannelAddEventListenerRequest" | "privateChannelDisconnectRequest" | "privateChannelUnsubscribeEventListenerRequest" | "raiseIntentForContextRequest" | "raiseIntentRequest";

These could also be used to implement a type guard.

Could you add a usage example or two so that we can evaluate what the best (and easiest to maintain) solution is?


export function isAddContextListenerRequest(value: any): value is AddContextListenerRequest {
try {
Convert.addContextListenerRequestToJson(value);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we happy with this "brute force" check of the types. It would be much harder to split the Convert class up to allow us to just check the conformance of rhte value rather than just trying to convert it to a string (which does conformance checking anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the best place for this config / code? I really didn't know so just stuck it here. Please suggest a better place to move it to if required.

Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few thoughts. Happy to discuss at Thursday's meeting.

Comment on lines +31 to +32
"posttypegen-browser": "npm run generate-type-predicates",
"generate-type-predicates": "ts-node code-generation/generate-type-predicates.ts"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this is the only thing run 'post' typegen-browser you can probably combine these entries.

// see https://www.typescriptlang.org/tsconfig to better understand tsconfigs
"files": ["generate-type-predicates.ts"],
"compilerOptions": {
"module": "CommonJS",
Copy link
Contributor

@kriswest kriswest Oct 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this really need its own tsconfig.json, or could it use the existing one in the root? The only differences between the two tsconfig files appear to be:

  • "module": "CommonJS" vs. "module": "esnext"
  • the file filter.

AFAIK ts-node is quite happy to handle ESM...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure that I would have only added this as I was getting errors without but hopefully I can get it to work without if I have another go.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original one has an include filter that would have excluded your codegeneration directory: https://github.com/finos/FDC3/blob/1a21b027f506d9ed88d5e3a1d55be38ac03ae828/tsconfig.json#L3C25-L3C25

The only other difference is your files entry above - could it have been that it just wasn't applying to your file?

}
}

export type RequestMessage = AddContextListenerRequest | AddEventListenerRequest | AddIntentListenerRequest | BroadcastRequest | ContextListenerUnsubscribeRequest | CreatePrivateChannelRequest | EventListenerUnsubscribeRequest | FindInstancesRequest | FindIntentRequest | FindIntentsByContextRequest | GetAppMetadataRequest | GetCurrentChannelRequest | GetCurrentContextRequest | GetInfoRequest | GetOrCreateChannelRequest | GetUserChannelsRequest | HeartbeatAcknowledgementRequest | IntentListenerUnsubscribeRequest | IntentResultRequest | JoinUserChannelRequest | LeaveCurrentChannelRequest | OpenRequest | PrivateChannelAddEventListenerRequest | PrivateChannelDisconnectRequest | PrivateChannelUnsubscribeEventListenerRequest | RaiseIntentForContextRequest | RaiseIntentRequest;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing this, there is another way. We already generate 3 types that will only validate against these groupings:

  • AppRequestMessage,
  • AgentResponseMessage,
  • AgentEventMessage

and each has a union of the type field string values that apply:

  • RequestMessageType,
  • ResponseMessageType,
  • EventMessageType

E.g.

export interface AppRequestMessage {
/**
* Metadata for a request message sent by an FDC3-enabled app to a Desktop Agent.
*/
meta: AppRequestMessageMeta;
/**
* The message payload typically contains the arguments to FDC3 API functions.
*/
payload: { [key: string]: any };
/**
* Identifies the type of the message and it is typically set to the FDC3 function name that
* the message relates to, e.g. 'findIntent', with 'Request' appended.
*/
type: RequestMessageType;
}
/**
* Metadata for a request message sent by an FDC3-enabled app to a Desktop Agent.
*/
export interface AppRequestMessageMeta {
requestUuid: string;
/**
* Field that represents the source application that a request or response was received
* from. Please note that this may be set by an app or Desktop Agent proxy for debugging
* purposes but a Desktop Agent should make its own determination of the source of a message
* to avoid spoofing.
*/
source?: AppIdentifier;
timestamp: Date;
}
/**
* Identifies the type of the message and it is typically set to the FDC3 function name that
* the message relates to, e.g. 'findIntent', with 'Request' appended.
*/
export type RequestMessageType = "addContextListenerRequest" | "addEventListenerRequest" | "addIntentListenerRequest" | "broadcastRequest" | "contextListenerUnsubscribeRequest" | "createPrivateChannelRequest" | "eventListenerUnsubscribeRequest" | "findInstancesRequest" | "findIntentRequest" | "findIntentsByContextRequest" | "getAppMetadataRequest" | "getCurrentChannelRequest" | "getCurrentContextRequest" | "getInfoRequest" | "getOrCreateChannelRequest" | "getUserChannelsRequest" | "heartbeatAcknowledgementRequest" | "intentListenerUnsubscribeRequest" | "intentResultRequest" | "joinUserChannelRequest" | "leaveCurrentChannelRequest" | "openRequest" | "privateChannelAddEventListenerRequest" | "privateChannelDisconnectRequest" | "privateChannelUnsubscribeEventListenerRequest" | "raiseIntentForContextRequest" | "raiseIntentRequest";

These could also be used to implement a type guard.

Could you add a usage example or two so that we can evaluate what the best (and easiest to maintain) solution is?

@Roaders
Copy link
Contributor Author

Roaders commented Oct 17, 2024

The reason for RequestMessage is to support Discriminated unions:

public onMessage(message: RequestMessage): void{
    switch(message.type){
         case: "raiseIntentRequest":
                return this.handleRaiseIntentRequest(message); // here message is typed as RaiseIntentRequestMessage rather than RequestMessage
        case "raiseIntentForContextRequest":
                return this.handleRaiseIntentForContextRequest(message); // message again correctly typed
        default:
                const unhandled: never = message.type; // compiler error if we have not handled all messages
                console.log(`Unknown message type encountered: ` + message.type);
    }

}

private handleRaiseIntentRequest(message: RaiseIntentRequestMessage): void {}

private handleRaiseIntentForContextRequest(message: RaiseIntentForContextMessage): void {}

@Roaders
Copy link
Contributor Author

Roaders commented Oct 17, 2024

Notes from FDC3 for Web Browsers Discussion group 17th October 2024 #1391

It would be better if didn't rely on the interface name to build the union types. If we could instead check that a given interface is logically compatible with AgentResponseMessage, AgentEventMessage or AppRequestMessage and use that to build the union types that would be better.

@robmoffat
Copy link
Member

Hi @Roaders ,

I would like to merge this into the fdc3-for-web-impl branch, which will form the basis of the implementation for 2.2
Can we rebase this PR against that? I could probably have a go at it if you don't want to.

The PR for this is #1309, which also changes the FDC3 project to use a monorepo structure. Your wor would need to go into the packages/fdc3-schema module, which generates BrowserTypes.ts. You might want to also have something in packages/fdc3-context which generates the various FDC3 standard context typescript definitions.

Let me know if you want to work on this, otherwise I might be able to take a look at it

@kriswest kriswest deleted the branch finos:fdc3-for-web-impl November 13, 2024 17:57
@kriswest kriswest closed this Nov 13, 2024
@robmoffat
Copy link
Member

@kriswest should this be closed?

@kriswest
Copy link
Contributor

No, not sure how I did that. I think it might have been automated after I deleted the branch it targeted.

@kriswest kriswest reopened this Nov 14, 2024
@kriswest kriswest changed the base branch from fdc3-for-web to main November 14, 2024 09:53
@kriswest
Copy link
Contributor

Re-opened and now pointed at main, could do with an update and npm run build or a rebase onto the fdc3-for-web-impl branch

@robmoffat
Copy link
Member

I'm going to merge this into fdc3-for-web-impl - turns out we need it there already!

@robmoffat robmoffat changed the base branch from main to fdc3-for-web-impl November 14, 2024 11:26
@robmoffat
Copy link
Member

merged manually into fdc3-for-web-impl-merging-prs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants