-
Notifications
You must be signed in to change notification settings - Fork 131
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
base: fdc3-for-web-impl
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for fdc3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
} | ||
} | ||
|
||
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; |
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.
These names RequestMessage
ResponseMessage
and EventMessage
probably need to change. I am open to suggestions!
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.
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.
Lines 528 to 564 in 6cf70dc
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); |
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.
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).
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.
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.
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.
Added a few thoughts. Happy to discuss at Thursday's meeting.
"posttypegen-browser": "npm run generate-type-predicates", | ||
"generate-type-predicates": "ts-node code-generation/generate-type-predicates.ts" |
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.
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", |
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.
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...
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 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.
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.
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; |
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.
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.
Lines 528 to 564 in 6cf70dc
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?
The reason for 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 {} |
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 |
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 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 Let me know if you want to work on this, otherwise I might be able to take a look at it |
@kriswest should this be closed? |
No, not sure how I did that. I think it might have been automated after I deleted the branch it targeted. |
Re-opened and now pointed at main, could do with an update and |
I'm going to merge this into fdc3-for-web-impl - turns out we need it there already! |
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.