fix: types from new changes in node toolkit[CNX-215]

[vikaskumr]

Purpose

Approach

Testing steps

Breaking Changes

Dependencies and/or References

Deployment

[vikaskumr]

We can have this approach or make a separate handler type for each handler.

[BasKiers]

I'm pretty sure this should not be necessary as we're not making use of any of the "CMA" specific context values in the handler.

[vikaskumr]

@BasKiers We are Getting TypeScript compilation errors when trying to use the function handlers:

Type 'FunctionEventDeliveryContext<InstallationParameters>' is not assignable to parameter of type 'FunctionEventDeliveryContext<Record<string, any>> & { cmaHost: string; uploadHost: string; appToken: string; cmaClientOptions?: any; cma?: PlainClientAPI | undefined; }'.

This is happening because LookUpHandler is intersection of Delivery and Management Context and typeScript couldn't guarantee that when routing to individual handlers, the context would have all required properties.

Basically this line in our public example app.

export type EventHandler = FunctionEventHandler<FunctionTypeEnum, InstallationParameters>;

Tldr is lookupHandler and SearchHandler have more restrictive requirements that the union type can't guarantee.

[vikaskumr]

We can solve this issue in lookupHandler by changing the & to | in node app toolkits but issue still remains in searchHandler

 [FunctionTypeEnum.ResourcesLookup]: {
    event: ResourcesLookupRequest
    context: FunctionEventManagementContext<P> | FunctionEventDeliveryContext<P>
    response: ResourcesLookupResponse
  }

[BasKiers]

Ok, I also tried quite some things but the main problem is that event and context are two separate function arguments and therefore Typescript has no ability to refine the type of context based on the refined type of event.

For now, I think this is the "best" solution we have other than making all properties in the ManagementContext optional (which would only make the DX worse for customers as they don't know what to expect from the context).

However, I would change this line to check for a property that is "required" in the ManagementContext type, e.g.

Suggested change

	if (event.type === 'resources.search' && 'cma' in context) {
	if (event.type === 'resources.search' && 'cmaHost' in context) {

[BasKiers]

I would rather use the context type

Suggested change

	export const getMockShopUrl = (context: { appInstallationParameters?: Record<string, any> }) => {
	export const getMockShopUrl = (context: FunctionEventDeliveryContext<InstallationParameters>) => {

or refactor the function to

Suggested change

	export const getMockShopUrl = (context: { appInstallationParameters?: Record<string, any> }) => {
	export const getMockShopUrl = (parameters: InstallationParameters) => {

[vikaskumr]

Closing it currently since this can be a breaking change for customers.