finos / FDC3

An open standard for the financial desktop.
https://fdc3.finos.org
Other
202 stars 132 forks source link

Updates from impl review #1424

Open julianna-ciq opened 2 weeks ago

kriswest commented 5 days ago

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:

robmoffat commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

@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 commented 3 days ago

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.