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

Modify generate-type-predicates to only include interfaces that have … #1444

Conversation

Roaders
Copy link
Contributor

@Roaders Roaders commented Nov 21, 2024

Describe your change

Modification of the generate-type-predicates code to build unions types of interfaces not based on the interface name but based on the type declared in the interface. If type extends the RequestMessageType union then the interface is included in the RequestMessage union

Related Issue

Contributor License Agreement

  • I acknowledge that a contributor license agreement is required and that I have one in place or will seek to put one in place ASAP.

Review Checklist

  • Issue: If a change was made to the FDC3 Standard, was an issue linked above?
  • CHANGELOG: Is a CHANGELOG.md entry included?
  • API changes: Does this PR include changes to any of the FDC3 APIs (DesktopAgent, Channel, PrivateChannel, Listener, Bridging)?
    • Docs & Sources: If yes, were both documentation (/docs) and sources updated?

      JSDoc comments on interfaces and types should be matched to the main documentation in /docs
    • Conformance tests: If yes, are conformance test definitions (/toolbox/fdc3-conformance) still correct and complete?

      Conformance test definitions should cover all required aspects of an FDC3 Desktop Agent implementation, which are usually marked with a MUST keyword, and optional features (SHOULD or MAY) where the format of those features is defined
    • Schemas: If yes, were changes applied to the Bridging and FDC3 for Web protocol schemas?

      The Web Connection protocol and Desktop Agent Communication Protocol schemas must be able to support all necessary aspects of the Desktop Agent API, while Bridging must support those aspects necessary for Desktop Agents to communicate with each other
      • If yes, was code generation (npm run build) run and the results checked in?

        Generated code will be found at /src/api/BrowserTypes.ts and/or /src/bridging/BridgingTypes.ts
  • Context types: Were new Context type schemas created or modified in this PR?
    • Were the field type conventions adhered to?
    • Was the BaseContext schema applied via allOf (as it is in existing types)?
    • Was a title and description provided for all properties defined in the schema?
    • Was at least one example provided?
    • Was code generation (npm run build) run and the results checked in?

      Generated code will be found at /src/context/ContextTypes.ts
  • Intents: Were new Intents created in this PR?

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.

Sorry, something went wrong.

@Roaders Roaders requested a review from a team as a code owner November 21, 2024 14:14
@kriswest
Copy link
Contributor

kriswest commented Nov 21, 2024

@Roaders it looks like everything is being generated twice (two copies of each type constant and is* function). Also can we discuss what's being generated here and the use cases again?

I've found the is* typeguard functions useful, but I am concerned that through the use of the convert functions, they may be more expensive than necessary and will return false (I think) if there are any excess properties... If they are for typing rather than full validation they could just be based on checking the type fields...

The message unions (intended to help create discriminated unions #1387 (comment)) seem to serve a similar role, as I could either do:

public onMessage(message: RequestMessage): void{
    switch(message.type){
         case: "raiseIntentRequest":
                ...
                break;
        case "raiseIntentForContextRequest":
                ...
                break;
        default:
                ...
                console.log(`Unknown message type encountered: ` + message.type);
    }

or

//Edit: removed, was not correct

The former would (currently) be more efficient. Do we need both?

I'm also concerned about the type constants generated and whether encouraging use of them is a mistake, adding complexity to code without benefit... They obscure the underlying values in code while not providing the usual benefit of a constant (a single place to set and change the value). Use of the generated types for messages would already confirm that the type field values are correct... The main thing we need (that the quicktype generated code doesn't give us) is a consistent, clean way to determine the type of an incoming message - where I believe this code currently provides us two competing methods of doing so.

I am aware that @robmoffat has the opposite opinion on use of the constants in code. We need to bring this to ground to progress the fdc3-for-web implementation.

Finally, perhaps we should just replace the schema-generated group types (e.g. AppRequestMessage) with their discriminated union counter parts (i.e. RequestMessage) to avoid duplication?

@kriswest kriswest changed the base branch from fdc3-for-web-impl-merging-prs to fdc3-for-web-impl November 21, 2024 17:00
@Roaders Roaders force-pushed the fdc3-for-web-impl-merging-prs branch from 339477c to 5ac033d Compare November 21, 2024 18:02
@Roaders
Copy link
Contributor Author

Roaders commented Nov 21, 2024

Have pushed some updates.

  • This should no longer duplicate code. It removes any existing statements of the same name before writing
  • This removes the interfaces for AppRequestMessage, AgentResponseMessage and AgentEventMessage and replaces them with the generated unions

I still have to create the additional validated type predicate which won't take long in the morning. Do we want to remove the constants? I'm afraid I didn't understand @robmoffat explanation of what they were for.

@Roaders
Copy link
Contributor Author

Roaders commented Nov 21, 2024

Hmmmm.... the diff is now huge... Could this just be because I deleted BrowserTypes and regenerated with the functions in a different order?

@Roaders Roaders force-pushed the fdc3-for-web-impl-merging-prs branch from 5ac033d to 524834b Compare November 22, 2024 08:45
@Roaders
Copy link
Contributor Author

Roaders commented Nov 22, 2024

public onMessage(message: AppRequestMessage): void{
    switch(message){
         case: isRaiseIntentRequest(message):
                ...
                break;
        case isRaiseIntentForContextRequest(message):
                ...
                break;
        default:
                ...
                console.log(`Unknown message type encountered: ` + message.type);
    }

@kriswest is the above valid typescript? I had never seen a switch statement like that (switching on the object then using type predicates for the cases). When I try it I get compile errors:

//this is fine
if (isAddIntentListenerRequest(message)) {
    return this.onAddIntentListenerRequest(message, sourceApp);
}

switch (message) {
    // this gives a compile error:
    // Type 'boolean' is not comparable to type 'AddContextListenerRequest | BroadcastRequest | CreatePrivateChannelRequest | FindInstancesRequest 
    case isAddEventListenerRequest(message):
        return this.onAddIntentListenerRequest(message, sourceApp);
}

@Roaders Roaders force-pushed the fdc3-for-web-impl-merging-prs branch from 524834b to 7979770 Compare November 22, 2024 11:11
@Roaders
Copy link
Contributor Author

Roaders commented Nov 22, 2024

Added the valid type predicates and sped up the normal predicate. Added some progress messages (generation is a bit slow). Added comments to type predicates.

@kriswest
Copy link
Contributor

@kriswest is the above valid typescript? I had never seen a switch statement like that (switching on the object then using type predicates for the cases). When I try it I get compile errors:

No its not valid - does narrow the type for you inside the cases, but doesn't make a correct comparison. My bad. A switch on message.type with the discriminated union and type strings in the cases should narrow the type inside the cases and check that the strings are valid - I'll test that later when back at my desk.

@kriswest
Copy link
Contributor

Hmmmm.... the diff is now huge... Could this just be because I deleted BrowserTypes and regenerated with the functions in a different order?

Its not that bad, looks to me mostly like the replacement union types, the is* functions and isValid* functions. The new functions LGTM based on a quick glance.

@Roaders
Copy link
Contributor Author

Roaders commented Nov 22, 2024

Hmmmm.... the diff is now huge... Could this just be because I deleted BrowserTypes and regenerated with the functions in a different order?

Its not that bad, looks to me mostly like the replacement union types, the is* functions and isValid* functions. The new functions LGTM based on a quick glance.

I undid my changes to the file and just ran the type predicates script on it rather than generating from fresh.

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.

LGTM

@Roaders
Copy link
Contributor Author

Roaders commented Nov 25, 2024

one of the checks is failing. what do I need to do to fixs that? is it test coverage?

@kriswest
Copy link
Contributor

@Roaders theres a test failing due to a typing issue (somethign that may need an adjustment after your changes) and then something is up with the code coverage reporting. I'll take a look at the first issue and see if I can resolve. TO do that I'm going to merge this PR into branch on the FINOS repo and then open a new PR.

@kriswest kriswest changed the base branch from fdc3-for-web-impl to fdc3-for-web-impl-update-type-predicates November 26, 2024 15:02
@kriswest kriswest merged commit 0bda0f1 into finos:fdc3-for-web-impl-update-type-predicates Nov 26, 2024
2 of 3 checks passed
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.

None yet

2 participants