Refactors ContextTypes&Intent to union type instead of enum

[andreifloricel]

closes #1138

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: andreifloricel / name: Andrei Floricel (17cf9e6, 6c0128f, e5449d7, f3e8d08, a19a633, 87b859e, df805e9, 265eed8, 972ff91)

[netlify]

✅ Deploy Preview for fdc3 ready!

Name	Link
🔨 Latest commit	17cf9e6
🔍 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.

[kriswest]

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.

[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)

[andreifloricel]

I tried to "trick" TypeScript to compile this, to no avail.
And TSDX seems to be more or less dead...

[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.

[bingenito]

Confirmed backcompat issue I raised is fixed. Looks good.

[kriswest]

This PR when in without a changelog entry, which we will need to add before the next version of FDC3 is adopted