Updates from impl review #1424

julianna-ciq · 2024-11-08T14:23:09Z

No description provided.

kriswest

This looks like a solid set of changes that will reduce the number of warnings from linting through better typing. I'll make a couple of quick changes to deal with some of my comments.

@robmoffat once done, I think you should merge this. I'll need to deal with some conflicts in my branch thereafter.

packages/addon/src/intent_resolver.ts

kriswest · 2024-11-20T14:17:29Z

packages/fdc3-agent-proxy/src/Messaging.ts

@@ -1,6 +1,6 @@
 import { Connectable, AppIdentifier, ImplementationMetadata } from "@kite9/fdc3-standard";
 import { RegisterableListener } from "./listeners/RegisterableListener";
-import { AddContextListenerRequest, AddContextListenerRequestMeta } from "@kite9/fdc3-schema/generated/api/BrowserTypes";
+import { AddContextListenerRequestMeta } from "@kite9/fdc3-schema/generated/api/BrowserTypes";


Only top-level types should be imported from BrowserTypes (as the naming of the lower-level types can be unstable),. Hence, I'd import AppRequestMessage and then extract the type for its meta element:

import { AppRequestMessage} from "@kite9/fdc3-schema/generated/api/BrowserTypes"; type RequestMetadata = AppRequestMessage["meta"];

then use that down on line 33.

packages/fdc3-agent-proxy/src/channels/DefaultChannel.ts

kriswest · 2024-11-20T14:23:46Z

packages/fdc3-agent-proxy/src/messaging/AbstractMessaging.ts

            (m.type == expectedTypeName)
            && (m.meta.requestUuid == message.meta.requestUuid), errorMessage)
        this.post(message)
-        const out: any = await prom
+        const out = await messageResponseWaiter
+        // @ts-expect-error: TODO: We should make a list of all of the possible messages that can be called through this, and make that list the generic. This will ensure better type safety.


I'm pretty sure that relevant types already exist for that: AppRequestMessage, AgentResponseMessage, AgentEventMessage were intended for such usage and contain unions of the relvant type strings for messages in their set.

Those types can't be easily applied yet due to these functions serving both WCP and DACP - I'll fix this in my branch where I've unpicked that already

kriswest · 2024-11-20T14:25:06Z

packages/fdc3-agent-proxy/src/messaging/AbstractMessaging.ts

-    getImplementationMetadata(): Promise<ImplementationMetadata> {
-        return Promise.resolve(this.implementationMetadata!!)
+    async getImplementationMetadata(): Promise<ImplementationMetadata> {
+        if(this.implementationMetadata === null){


Reasonable to throw an error - where I hit this in another branch I generated impl metadata with "unknown" values. It shouldn't happen, but it pays to be defensive and to handle the error if it ever comes up.

kriswest · 2024-11-20T14:31:54Z

packages/fdc3-agent-proxy/src/messaging/AbstractMessaging.ts

+        if(this.implementationMetadata === null){
+            throw new Error("Unconnected. No metadata available.")
+        }
+        return this.implementationMetadata
    }

    private async exchangeValidationWithId<X>(message: any, connectionAttemptUuid: string): Promise<X> {


I'm moving the identity validation into the Loaders and out of here in another branch. Its part of the connection flow for Web and should be separated from messaging (that could be used with a different connection).

kriswest · 2024-11-20T14:33:55Z

packages/fdc3-get-agent/src/strategies/TimeoutLoader.ts

@@ -26,9 +26,9 @@ export class TimeoutLoader implements Loader {
        this.done = true;
    }

-    get(params: GetAgentParams): Promise<DesktopAgent | void> {
+    get({timeoutMs = 0}: GetAgentParams): Promise<DesktopAgent | void> {


There is a constant for the default timeout elsewhere - but I'm also eliminating the timeoutLoader in another branch (with id validation moving into the loader, loaders can do some ops outside of the timeout so they need to manage the timeouts themselves

packages/fdc3-get-agent/src/ui/DefaultDesktopAgentChannelSelector.ts

packages/fdc3-get-agent/test/step-definitions/intent-resolver.steps.ts

toolbox/fdc3-for-web/demo/src/client/ui/channel-selector.ts

…tor.ts

…eview-updates

kriswest · 2024-11-20T16:11:41Z

Resovled conflicts @robmoffat - this should be ready for you.

One thing to watch out for: don't import the various generated types in BrowserTypes other than the top-level message types, as the naming of the autogenerated types may be unstable. That also goes for constants used for message type fields. Instead you can:

Apply a top-level message type to a whole message - it will highlight if the string type is wrong and help you correct
Use 'indexed access types' for typing of properties. E.g. if you just want the payload of a message type then do:
```
import { Fdc3UserInterfaceRestyle } from "@kite9/fdc3-schema/dist/generated/api/BrowserTypes";
type RestylePayload = Fdc3UserInterfaceRestyle ["payload"]
```
That way you are not dependent on the auto-generated sub-types.

TypeError: Class extends value undefined is not a constructor or null at Object.<anonymous> (/home/runner/work/FDC3/FDC3/packages/fdc3-get-agent/src/ui/DefaultDesktopAgentIntentResolver.ts:11:1)

robmoffat · 2024-11-22T13:44:45Z

Have you tried running the tests on this @kriswest?

I get this error (consistent with the coverage run):

TypeError: Class extends value undefined is not a constructor or null
    at Object.<anonymous> (/home/runner/work/FDC3/FDC3/packages/fdc3-get-agent/src/ui/DefaultDesktopAgentIntentResolver.ts:8:1)
    at Module._compile (node:internal/modules/cjs/loader:1469:14)
    at Module.replacementCompile (/home/runner/work/FDC3/FDC3/node_modules/append-transform/index.js:60:13)
    at Module.m._compile (/home/runner/work/FDC3/FDC3/node_modules/ts-node/src/index.ts:1618:23)
    at module.exports (/home/runner/work/FDC3/FDC3/node_modules/default-require-extensi

This might be fixable by merging main back into the branch, perhaps?

kriswest · 2024-11-22T14:14:08Z

Have you tried running the tests on this @kriswest?

Yes, I don't know why thats happening. I merged main into this branch a couple of days ago which is actually when this started. Its an issues with a class trying to extend something that it can't find (which is therefore undefined). That could be an error in the codebase - but it probably should blow up on compile if thats the case (although the current build is not bundling as its just using tsc), or it could be an issue with a dependency...

kriswest · 2024-11-22T15:27:51Z

@robmoffat found it! It was the import of FDC3_VERSION in packages/fdc3-get-agent/src/ui/AbstractUIComponent.ts. The PR fixes an incorrect payload in an Fdc3UserInterfaceHandshake message by adding the missing FDC3 version number, but creates a circular dependency.

I've corrected the dependency by moving the FDC3 version into its own file - although that should probably be moved to the top of the dependency chain (fdc3-standard package)? That then revelaed some other issues (unused imports in tests). I've corrected those and now it runs, but seems to get stuck, its also timing out in a few tests - I'm not sure why. Could you run the tests and see if you can replicate? I may be on a different node version to you (20.11.1) which might make a difference

kriswest · 2024-11-22T15:34:30Z

P.S. It gets stuck at the end of the tests (for the fdc3-get-agent package) I believe. Its just not exiting. Its already printed the summary.

julianna-ciq added 7 commits November 8, 2024 09:14

Add a needed dep

58c026d

Remove unused file

8d2b23f

Remove commented-out code

3a0fc4e

Update clean script

ec18f6b

Remove unused line

0b158fb

Clean up some types in light of finos#1275

576f091

Use console.debug

8934214

julianna-ciq requested a review from a team as a code owner November 8, 2024 14:23

Merge branch 'fdc3-for-web-impl' into impl-review-updates

f432231

julianna-ciq mentioned this pull request Nov 8, 2024

FDC3 For Web Browsers Implementation DO NOT MERGE OR REVIEW #1309

Open

julianna-ciq added 20 commits November 8, 2024 09:31

Refactor to reduce nesting

a7ca5a9

Fix build issue

5dc1177

Clarify a comment

86c6c3b

Update AbstractMessaging.ts

3ed4fe3

test error

c8bb7a0

Refactor file MessagePortMessaging.ts

c3252ef

Update script

ec54969

build

555886d

Remove commented out code

020ea95

Refactor

9ef87c8

Move reused Given to Background

d45c41b

Present tense in steps

3418420

Review changes

135ec35

Comment out out-of-scope GHA steps

44bad3b

Minor tidying

f341426

Refactor for flow control and typings

64120b3

Tidying

b206737

Merge branch 'fdc3-for-web-impl' into impl-review-updates

a4a6521

Add npx to package.json

7945595

Remove 'old' folder

cb38606

julianna-ciq added 2 commits November 12, 2024 10:30

Tidying

0dbe939

tidy

aadcb58

kriswest approved these changes Nov 20, 2024

View reviewed changes

kriswest added 4 commits November 20, 2024 15:06

Update packages/fdc3-get-agent/src/ui/DefaultDesktopAgentChannelSelec…

4f7b6be

…tor.ts

Dealing with review comments

c3e7e7b

Merge remote-tracking branch 'upstream/fdc3-for-web-impl' into impl-r…

09a90db

…eview-updates

Bridging schema fix and updating package-lock

022c4bc

kriswest added 2 commits November 20, 2024 16:20

addressing weird semgrep errors on log messages

0b18498

tweakng typing in hopes of fixing a circular dependency import...

ebc54d9

TypeError: Class extends value undefined is not a constructor or null at Object.<anonymous> (/home/runner/work/FDC3/FDC3/packages/fdc3-get-agent/src/ui/DefaultDesktopAgentIntentResolver.ts:11:1)

kriswest mentioned this pull request Nov 20, 2024

FDC3 for Web Browsers Discussion group #1443

Open

7 tasks

reenable code coverage

786cbdb

Correcting issues with tests (a circular dependency and unused imports)

eec9284

robmoffat merged commit 887b26a into finos:fdc3-for-web-impl Dec 3, 2024
3 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates from impl review #1424

Updates from impl review #1424

julianna-ciq commented Nov 8, 2024

kriswest left a comment

kriswest Nov 20, 2024

kriswest Nov 20, 2024

kriswest Nov 20, 2024

kriswest Nov 20, 2024

kriswest Nov 20, 2024

kriswest Nov 20, 2024

kriswest commented Nov 20, 2024

robmoffat commented Nov 22, 2024

kriswest commented Nov 22, 2024

kriswest commented Nov 22, 2024

kriswest commented Nov 22, 2024

Updates from impl review #1424

Updates from impl review #1424

Conversation

julianna-ciq commented Nov 8, 2024

kriswest left a comment

Choose a reason for hiding this comment

kriswest Nov 20, 2024

Choose a reason for hiding this comment

kriswest Nov 20, 2024

Choose a reason for hiding this comment

kriswest Nov 20, 2024

Choose a reason for hiding this comment

kriswest Nov 20, 2024

Choose a reason for hiding this comment

kriswest Nov 20, 2024

Choose a reason for hiding this comment

kriswest Nov 20, 2024

Choose a reason for hiding this comment

kriswest commented Nov 20, 2024

robmoffat commented Nov 22, 2024

kriswest commented Nov 22, 2024

kriswest commented Nov 22, 2024

kriswest commented Nov 22, 2024