finos / FDC3

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

Refactoring ContextTypes to union type (instead of enum) #1138

Closed andreifloricel closed 2 months ago

andreifloricel commented 6 months ago

Currently the standard FDC3 Context types are defined as an enum: https://github.com/finos/FDC3/blob/43ce7b2c17b80b9117a339950789867641ea0929/src/context/ContextType.ts#L5-L25

This causes problems when constructing mapped and/or conditional types, e.g. generically typing a function so that for a given ContextType the corresponding Context structure is returned (fdc3.chart'` -> `Chart`,fdc3.action'->Action`, etc.).

The ContextType defined here is not helping, as it's NOT a valid union type: https://github.com/finos/FDC3/blob/43ce7b2c17b80b9117a339950789867641ea0929/src/context/ContextType.ts#L27

Also, in current versions of TypeScript everything enum types offer can be achieved with union types (and much more) without the inherent problems that TypeScript Enums have: https://www.typescriptlang.org/docs/handbook/enums.html#const-enum-pitfalls

My suggestion is to replace the current implementation with a simple union type. Possibly also clearly differentiate between standard FDC3 context types and custom ones:

export type StandardContextTypes =
  | 'fdc3.action'
  | 'fdc3.chart'
  | 'fdc3.chat.initSettings'
  | 'fdc3.chat.message'
  | 'fdc3.chat.room'
  | 'fdc3.chat.searchCriteria'
  | 'fdc3.contact'
  | 'fdc3.contactList'
  | 'fdc3.country'
  | 'fdc3.currency'
  | 'fdc3.email'
  | 'fdc3.instrument'
  | 'fdc3.instrumentList'
  | 'fdc3.interaction'
  | 'fdc3.message'
  | 'fdc3.organization'
  | 'fdc3.portfolio'
  | 'fdc3.position'
  | 'fdc3.nothing'
  | 'fdc3.timerange'
  | 'fdc3.transactionResult'
  | 'fdc3.valuation';

export type ContextTypes = StandardContextTypes | string;

Currently, I need to define these types in all applications that interact with FDC3, but it would be nice to have them in the main library.

kriswest commented 5 months ago

I've had similar trouble using the TS enums and migrated everything else (in generated code) to string unions recently. Hence, I support this. We had one question raised about whether this should even be in the NPM module at all (i.e. whether its actually useful at runtime). An alternative might be to make this an array that can more easily be used for a runtime check... See https://stackoverflow.com/questions/36836011/checking-validity-of-string-literal-union-type-at-runtime

If we make this change, we should probably advise on use. Perhaps even include a utility function that checks whether a context is a standard type in the API's Methods.ts or in the ContextType.ts file...