Fdc3 for web impl get agent refactor #1466
Conversation
Individual timeouts allow for steps that our outside the timeout (identity validation)
… (which needs further work)
ImplementationMetadata is now passed into AbstractMessaging's constructor and getting close to eliminating AbstractWebMessaging. Correcting typing and generics on a number of functions (replacing any and unqualified generics)
…web-impl-getAgent-refactor
- Eliminate AbstractWebMessaging - Messaging is no longer a Connectable (as the connection is provided to it - was previously a no-op) - Handling getInfo via a message exchange rather than passing down through connection flow - Messaging implementations no longer handle implementationMetadata, instead holding the AppIdentifier directly - Renamed HandshakeSupport HeartbeatSupport as its no longer involved in holding implementaiton metadata. - Improving typing and error handling (defensive coding) in get-agent-proxy
…-web-impl-getAgent-refactor
|
|
|
awesome, I'll try and get to it first thing on Monday |
| From picocolors (imported due to issues building in both ESM and CommonJS) | ||
| https://github.com/alexeyraspopov/picocolors | ||
|
|
||
| ISC License |
There was a problem hiding this comment.
I'm a bit worried about this license. A lot of FDC3's users have automated tools for license scanning and approvals. If FDC3 suddenly ends up being mixed-license (Apache + ISC) then it may get flagged.
According to opensource.org https://opensource.org/license/isc-license-txt. this license is currently not categorised as open source. Is there an alternative with an Apache license we can use?
There was a problem hiding this comment.
I intend to remove it and replace with a custom implementation - this was a result of my frustration at not finding a lib for this that would build under both es6 and commonjs. I was going to do that in the debug logs PR...
There was a problem hiding this comment.
Actually, it is an OpenSource license, approved by the Open Source Initiative - the categorizations they display are within the set of Open Source licenses and are:
Its also the default license on NPM packages created with npm init (I'm told).
Regardless I intend to replace this with a custom implementation.
There was a problem hiding this comment.
ok great. Yes, I misunderstood that.
There was a problem hiding this comment.
picocolors removed / replaced
| @@ -161,6 +161,10 @@ export function setupGenericSteps() { | |||
| expect(handleResolve(field, this)['message']).toBe(errorType); | |||
| }); | |||
|
|
|||
| Then('{string} is an error', function (this: PropsWorld, field: string) { | |||
| expect(handleResolve(field, this)).toBeInstanceOf(Error); | |||
There was a problem hiding this comment.
Why not use the existing 'error with message' and assert that the error matches "Invalid arguments passed to addContextListener!"?
There was a problem hiding this comment.
Because the error message is non-standardized. There doesn't seem any point in enforcing the arbitrary message. Instead I'd rather pursue this in 2.3 and then do it properly:
There was a problem hiding this comment.
WIll add a comment to the test and code regarding that.
| */ | ||
| goodbye(instanceId: string): void; | ||
| setFDC3Server(server: FDC3Server): void; |
There was a problem hiding this comment.
rather than introduce the FDC3Server here, it might be better to have cleanup involve just the ServerContext and MessageHandler classes. setAppState() on the ServerContext can then chain cleanup on the MessageHandlers if it wants to.
There was a problem hiding this comment.
The problem is that the ServerContext doesn't have a reference to the handlers, nor do the handlers to each other and both can handle/cause disconnections that they need to notify each other about. The Server is only thing with references to both. As I said earlier I don't understand the design decision in separating the ServerContext from the handlers as both have to hold state for applications and clean it up when appropriate.
If you don't want the ServerContext linked back to the server, then it will need to be linked to all the handlers so it can notify them to clean up their state (which is all that link is for) - that may be worse... This is a second attempt at solving the same state problem, if you want me to make a third then I need you to identify an architectural change that you'll accept AND that solves the state problem.
There was a problem hiding this comment.
yes, I agree about the problem. This is as a result of cleanup needing to work across all the handlers. The ServerContext is separated out from the handler as it's expected that an implementation of the Desktop agent Server (in this case, demo), will want to implement that in it's own way. Whereas the behaviour of opening / resolving intents etc. will be reused.
If you don't want the ServerContext linked back to the server, then it will need to be linked to all the handlers so it can notify them to clean up their state (which is all that link is for) - that may be worse...
I think it would be less bad to have two classes referencing each other than three. But if you don't think this would be an improvement then just leave it.
There was a problem hiding this comment.
Well... it wouldn't be 3 classes so much as 1 <-> 4 (each handler would need to reference the context and vice-versa and custom implementations would have to implement the link.
What I mean on the architecture is that the ServerContext could hold the references to the handlers instead of the server... And if we do that we might as well merge the concepts of the FDC3Server and ServerContext as theres not a lot of code in the server... Are we deriving a benefit from keeping them separate? I count something like 5 lines of code we get from the BasicFDC3Server...
| @@ -128,7 +128,7 @@ const openChannelIframe = (e: MouseEvent) => { | |||
| const channel = new MessageChannel(); | |||
|
|
|||
| // STEP 2B: Receive confirmation over port from iframe | |||
| channel.port1.onmessage = ({ data }) => { | |||
| channel.port1.addEventListener('message', ({ data }) => { | |||
There was a problem hiding this comment.
Have you tested this code? I can't get it to run. If you can't either I suggest we take it out and the index.html file - people will be able to test the channel selector and intent resolver much better through the demo.
There was a problem hiding this comment.
It looked well out of date to me and out of sync with the UI implementations, I fixed a few things but not tested - it definitely needs work to run.
This comment was marked as outdated.
This comment was marked as outdated.
|
Ok, one more UI thing, the default channel appears to be white now, instead of black. But that makes it hard to see against the workbench: 
blue channel looking fine 
no channel is invisible Obviously, if workbench was running in dark mode, the reverse would be true! We might want to consider adding a white background to the channel icon? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Conformance tests all passing for me. 🥳 |
|
|
1 similar comment
|
|
Oh you have to start the server for the reference UI separately... Still no white or black channels however. Also neither UI seems to allow deselecting the channel, which seems a flaw. |
No, it's supposed to start: That part worked for me. The bit that didn't work so well that I drew attention to in my review is the index.html page in this ui-reference project, which doesn't really do anything, and the main.ts file that it uses. Still no white or black channels however.
Agreed |
I believe the purpose was testing outside of the DA, not sure when it fell behind.
Ah you mean when no channel is selected! I'll work on making it possible to deselect the channel first then have a think, we probably need styling that will work on either black or white... perhpas a white stroke on the cube + a drop shadow? |
Yes, sorry. It's clear in the demo's own implementation but not visible in the reference-ui version. I'll leave you to exercise your design skills 😁 |
|
It wasn't possible to deselect the channel in either implementation, I've fixed that as well as text contrast on each item: Demo: Default: And have tried to bodge the logo styles on the default implementation to be visible on any backdrop:
Although it may be better to use the color contrast function I added for the text colours to flip the stroke color to black when white doesn't contrast with the colour... That'll need a little tuning of the SVG to look right. |
|
|
|
@robmoffat should be ready for another look. I've not touched the index.html in reference-ui, but am otherwise done I think. |
|
|
| name: 'Channel 1', | ||
| color: 'red', | ||
| glyph: '1', | ||
| glyphColor: 'white', |
There was a problem hiding this comment.
I'm getting a compile error here, which I can fix by changing to this:
export type ChannelState = {
id: string;
type: ChannelType;
context: Context[];
displayMetadata: DisplayMetadata & { glyphColor: string };
};
in BroadcastHandler
but is that just me?
There was a problem hiding this comment.
... but actually I don't think you're even using glyphColor!
There was a problem hiding this comment.
Gah, you're right. That was an abortive attempt to fix color contrast on channels - before I went and did it properly with a luminance based contrast calculation. Removed it. Getting no build errors this end.
|
|
|
do you want to merge this? |
Describe your change
Significant refactor of the fdc3-get-agent package to align it with the Standard and a number of change fdc3-agent-proxy or fdc3-web-impl that were necessitated by the changes.
Changes:
BasicDesktopAgenttoDesktopAgentProxyto better represent its role.anytypes and type overrides in many places - there are others still to addressgetAgentimplementation considering its own window to be a possible parent and to set up messaging with it 0t this is now preventedRelated Issue
Addresses a number of issues in addition to working to improve the implementation generally:
resolves #1429
resolves #1430
resolves #1433
resolves #1445
resolves #1468
Contributor License Agreement
Review Checklist
DesktopAgent,Channel,PrivateChannel,Listener,Bridging)?JSDoc comments on interfaces and types should be matched to the main documentation in /docs
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
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
npm run build) run and the results checked in?Generated code will be found at
/src/api/BrowserTypes.tsand/or/src/bridging/BridgingTypes.tsBaseContextschema applied viaallOf(as it is in existing types)?titleanddescriptionprovided for all properties defined in the schema?npm run build) run and the results checked in?Generated code will be found at
/src/context/ContextTypes.ts