Refactors ContextTypes&Intent to union type instead of enum

[andreifloricel]

closes #1138

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: andreifloricel / name: Andrei Floricel (17cf9e62ad1a87d7faadc993f0cfe4ea67d852ac, 6c0128fde7a73e1cc11aedc21f1775dbadd5fc38, e5449d74de93d50e931eea7177f0573793435b9f, f3e8d084b250b55cf3749ad80ee0deaa46392e75, a19a6335e03b249eaaab9ce4ea12601dd3e5b362, 87b859ee3dae81fff00e56108f8c7f4b2b066e3d, df805e9390fdd2c2f9b0d7d8c9741ac8072f6076, 265eed8dbddc5bfc3b9c90e1b30b6a63fde81144, 972ff91bfd97ca8d29d3d1f253349e29729853a5)

[netlify[bot]]

✅ Deploy Preview for fdc3 ready!

Name	Link
🔨 Latest commit	17cf9e62ad1a87d7faadc993f0cfe4ea67d852ac
🔍 Latest deploy log	https://app.netlify.com/sites/fdc3/deploys/65f00cb0d36cdf0008cc230b
😎 Deploy Preview	https://deploy-preview-1139--fdc3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[andreifloricel]

Sorry, this PR has slipped under my radar over the last few weeks. I'll update it with suggested improvements in the coming days.

[bingenito]

This change LGTM - although I think we should consider whether to add an array or use an array instead to simplify implementation of a runtime check as to whether a context is a standard type.

@kriswest Do you have a use case in mind where you would need to know if it is a standard type?

[kriswest]

@andreifloricel could outline the use case for this type and confirm or deny whether you see a use case for a util function to check if a context is a standard type at runtime? If we can confirm that I can probably document it and add in a util fn to do it (which may need an array or the type names).

Without a use case, one of the other maintainers suggested dropping the enum/union entirely... I sort of see a use case - but I'd much rather confirmation from someone using it for real!

[andreifloricel]

@andreifloricel could outline the use case for this type and confirm or deny whether you see a use case for a util function to check if a context is a standard type at runtime?

I think it all comes down to a type-safe API and implicitly to a superior DX. E.g. it's quite nice to get have autocompletion for Context/Intents on all the DesktopAgent methods. Also checking for standard Context/Intent is quite useful when your implementation has to support dynamic subscribing/emitting of FDC3 payloads (e.g. raiseIntentForContext() without knowing upfront which Context will the user provide, etc)

@kriswest I updated the PR:

• refactored both standard Context and Intent to Union Types
• adjusted all (I hope I got all of them) Intent references from string to Intent

• added utility functions: isStandardContextType(contextType: string),isStandardIntent(intent: string),getPossibleContextsForIntent(intent: StandardIntent)

  Note:

  • I intentionally kept the public types for Intent&ContextType "clean" and readable and put the internal types and configurations in IntentsConfiguration.ts
  • nevertheless, the public types (Union Types) are the single source of truth: once you add/remove/adjust something there, the compilation fails until the IntentsConfiguration.ts is updated accordingly
  • TSDX is stuck on a very old TypeScript version which doesn't support newer syntax like satisfies

  L.E. I thought I managed to circumvent the TS problem when I finally managed to commit the changes but it was a false positive, in the end I had to comment out that part (see my comment in IntentsConfiguration.ts)

[bingenito]

Isn't this a semver major breaking api change to anyone using the old style constants like below?

  import { ContextTypes, Intents } from "@finos/fdc3";
  window.fdc3.addContextListener(ContextTypes.Instrument, () => {});
  window.fdc3.addIntentListener(Intents.ViewInstrument, () => {});

[bingenito]

• added utility functions: isStandardContextType(contextType: string), `isStandardIntent(intent: string)

I don't understand the use case for these. Do you have a local implementation dependent on these?

getPossibleContextsForIntent(intent: StandardIntent)

Isn't this opinionated?

[andreifloricel]

Isn't this a semver major breaking api change to anyone using the old style constants like below?

  import { ContextTypes, Intents } from "@finos/fdc3";
  window.fdc3.addContextListener(ContextTypes.Instrument, () => {});
  window.fdc3.addIntentListener(Intents.ViewInstrument, () => {});

No, all changes in this PR are backward compatible:

• ContextType and Intent types still accept any string value ( as it must support custom Contexts/Intent)
• also, changes in the DesktopAgent Api don't affect existing implementations, see https://tsplay.dev/wX0KLw
• the main advantages over the previous implementation are autocompletion and and significantly better TypeScript flexibility (mapped types, type guards, etc)

[andreifloricel]

• added utility functions: isStandardContextType(contextType: string), `isStandardIntent(intent: string)

I don't understand the use case for these. Do you have a local implementation dependent on these?

A real use case for us is that we need to dynamically build UI controls for our users (e.g., right-click a grid cell and generate an intent for the context referenced in that specific cell). When we do this, it's very useful to know if that specific intent and/or context is a standard FDC3 one, because in that case we can make a lot of educated guesses. Such as, for example, deriving possible contexts for a standard intention (see the next section of the commentary). @kriswest what use case did you have in mind?

getPossibleContextsForIntent(intent: StandardIntent)

Isn't this opinionated?

Well, it is, but it's FDC3's opinion :) As I understand the FDC3 specification, each standard intent (Verb) has an associated list of possible contexts (Nouns), e.g. https://fdc3.finos.org/docs/intents/ref/ViewChart#possible-contexts

[bingenito]

Isn't this a semver major breaking api change to anyone using the old style constants like below?

  import { ContextTypes, Intents } from "@finos/fdc3";
  window.fdc3.addContextListener(ContextTypes.Instrument, () => {});
  window.fdc3.addIntentListener(Intents.ViewInstrument, () => {});

No, all changes in this PR are backwards compatible:

• ContextType and Intent types still accept any string value ( as it must support custom Contexts/Intent)
• also, changes in the DesktopAgent Api don't affect existing implementations, see https://tsplay.dev/wX0KLw

I agree with the change and type guard, but it is a breaking change on the caller side as ContextTypes and Intents is no longer exported. Put the above in a new file in the project and it is fine. Switch to this branch and it has errors for missing exports.

[andreifloricel]

I agree with the change and type guard, but it is a breaking change on the caller side as ContextTypes and Intents is no longer exported.

Good spot! I reintroduced the deprecated enum types but marked them as deprecated.

[kriswest]

Shall we pick this up briefly at tomorrow's Standards Working Group meeting? Happy to add it to the agenda - I haven't yet caught up on the conversation.

[andreifloricel]

Hi @kriswest,

since I can't attend today's Standards Working Group meeting, I'll try to to summarize all the changes in this task:

• marked existing ContextTypes and Intents enums as deprecated
• introduced new union types ContextType and Intent (supporting both standard and custom values)
• adjusted all DesktopAgent Intent and ContextType references from to the new union types
• added utility functions:
  • isStandardContextType(contextType: string)
  • isStandardIntent(intent: string)
  • getPossibleContextsForIntent(intent: StandardIntent)

Note:

• I intentionally kept the public types for Intent&ContextType "clean" and readable and put the internal types and configurations in IntentsConfiguration.ts
• nevertheless, the public types (Union Types) are the single source of truth: once you add/remove/adjust something there, the compilation fails (should fail, but it does NOT, see next point) until the IntentsConfiguration.ts is updated accordingly
• because TypeScript version is quite old and TSDX library is more or less dead and unmaintained, I couldn't type the Intent to Context mapping correctly (using the satisfies construct); it's a matter of time before other things break down, I think migration to another tool should be considered (Vite?)

[andreifloricel]

I removed the getPossibleContextsForIntent() method.