FDC3 icon indicating copy to clipboard operation
FDC3 copied to clipboard

Erroneous Property / Structure for Channel Events

Open robmoffat opened this issue 8 months ago • 2 comments

Minor Issue

Please use minor issues for discrepancies or errors found in the spec or supporting documentation, examples, and other materials. If you wish to propose a new feature or a revision to an existing feature, please use the Enhancement Request template.

Area of Issue

  • [ ] App Directory
  • [x] API
  • [ ] Context Data
  • [ ] Intents
  • [ ] Desktop Agent Bridging
  • [ ] Use Cases
  • [ ] Other

This is a bit confusing:

// Events.ts

/**
 * Type defining the format of event `userChannelChanged` objects
 */
export interface FDC3ChannelChangedEvent extends FDC3Event {
  readonly type: 'userChannelChanged';
  readonly details: {
    currentChannelId: string | null;
  };
}
// BrowserTypes.ts

export interface ChannelChangedEvent {
  /**
   * Metadata for messages sent by a Desktop Agent to an app notifying it of an event.
   */
  meta: BroadcastEventMeta;
  /**
   * The message payload contains details of the event that the app is being notified about.
   */
  payload: ChannelChangedEventPayload;
  /**
   * 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 'Response' appended.
   */
  type: 'channelChangedEvent';
}

/**
 * The message payload contains details of the event that the app is being notified about.
 */
export interface ChannelChangedEventPayload {
  /**
   * The Id of the channel that the app was added to or `null` if it was removed from a
   * channel.
   */
  newChannelId: null | string;
}

the problem is that one is taking a newChannelId and the other is currentChannelId. ... and the event type changes too

ok, this is a weird issue, definitely an error:

/**
 * Type defining the format of event objects that may be received
 * via the FDC3 API's `addEventListener` function.
 */
export interface FDC3Event extends ApiEvent {
  readonly string: FDC3EventTypes;
  readonly details: any;
}

/**
 * Type defining the format of event `userChannelChanged` objects
 */
export interface FDC3ChannelChangedEvent extends FDC3Event {
  readonly type: 'userChannelChanged';
  readonly details: {
    currentChannelId: string | null;
  };
}

There's a property called "string" on the FDC3Event!

3:01 I'll raise an issue

Message Kris West

robmoffat avatar Apr 25 '25 14:04 robmoffat

The FDC3Event schema looks correct to me: https://github.com/finos/FDC3/blob/7abbe23703b1a07fca3d4d4495db845042da053f/packages/fdc3-schema/schemas/api/api.schema.json#L522-L539

The error is only in the typescript for FDC3Event: https://github.com/finos/FDC3/blob/7abbe23703b1a07fca3d4d4495db845042da053f/packages/fdc3-standard/src/api/Events.ts#L34-L37 and is probably not affecting anyone much as its not replicated on the subtypes: https://github.com/finos/FDC3/blob/7abbe23703b1a07fca3d4d4495db845042da053f/packages/fdc3-standard/src/api/Events.ts#L42-L47

Nevertheless its unintended and should be fixed. It is probably not affecting conformance...

As for the different field names, it looks like @kemerava and I were working on these in parallel. I think the message used in FDC3 for Web might predate us adding event listeners to the API (as the proxy and channel selectors needed to know about the channel change before apps could listen for it). Its implementable as is (but you have to translate the field name when you fire the event which is inelegant - just has to happen in proxy code which does the event firing on receipt of message so its handled for all implementations that use the FDC3 lib for getAgent - @Roaders take note as you don't use said lib).

To improve on it I suggest we add both fields to the message, deprecate newChannelId (preferring currentChannelId from the FDC3 API) and have the proxy code check for checkCurrentId then fallback to newChannelId if not present. The schema will need a little finesse with an anyOf so it can take either parameter but require one to be present (won't replicate in the typescript where both props will be optional but thats the best we can do).

Does that sound agreeable @robmoffat?

kriswest avatar Jun 04 '25 09:06 kriswest

Yes, this sounds like a good approach.

robmoffat avatar Jun 05 '25 15:06 robmoffat