pact-foundation / pact-js

JS version of Pact. Pact is a contract testing framework for HTTP APIs and non-HTTP asynchronous messaging systems.
https://pact.io
Other
1.62k stars 343 forks source link

fix: `stateHandlers` type in `VerifierOptions` #1120

Open ben-styling opened 12 months ago

ben-styling commented 12 months ago

Summary

This PR fixes the stateHandlers property on VerifierOptions by making the stateHandlers on MessageStateHandlers a generic type, which is set as unknown in the MessageProviderOptions type.

This issue is described in #1057, and marked as fixed in #1062, but the type issue still persists in v12.1.0

When using the following:

stateHandlers: {
  'some state': {
    setup: async () => {},
    teardown: async () => {},
  }
}

The following TS error is present

Type '{ setup: () => Promise<void>; teardown: () => Promise<void>; }' provides no match for the signature '(state: string, params?: { [name: string]: string; } | undefined): Promise<unknown>'.ts

image

I'm not very familiar with pact yet, this might be a breaking change as the MessageStateHandlers is exposed.

Would love to know if there's a better way to do this!

YOU54F commented 11 months ago

Thanks! I'm also not sure of the far reaching implications of how this will affect end typescript users or if there is a better way without doing a bit more background reading.

I'll put this on my list to get sorted asap. Is it possible to create a minimal example that shows the issue, and then we can see it resolved :)

TimothyJones commented 10 months ago

I'm afraid this fix isn't correct, for two reasons.

Firstly, the error message is right (although not super helpful). The type:

{
  setup: () => Promise<void>;
  teardown: () => Promise<void>;
}

isn't assignable to the MessageStateHandlers. MessageStateHandlers expects a setup function only, and doesn't support teardown - see here for how it gets used.

As mentioned on https://github.com/pact-foundation/pact-js/pull/1062, probably this should be addressed so that the types are the same and there is a setup and teardown function as in the http statehandlers.


Secondly, making it generic would mean that all state handlers need to return the same type, which wouldn't be convenient for users (or correct). It might be an improvement to make it implicitly generic - something like:

export interface MessageStateHandlers {
  [name: string]: <T>(
    state: string,
    params?: { [name: string]: string }
  ) => Promise<T>;
}

but since the return type is actually thrown away with message pacts, I think unknown is more appropriate.

Aside: It's probably a bug to throw away the return value - I suspect this was missed when message pacts were migrated to the rust core. This means that provider state variables won't work with message pacts.


Lastly, if you did merge this, it would be a breaking change, since anyone naming MessageStateHandlers would now need to specify the generic. You can mark a breaking change with fix!: (but make sure that you have commit message that explains what broke and how to address it).

I'm also unclear on how making this type generic allowed you to assign that object - with the change, even setting it to any still raises a compile error for me:

const foo: MessageStateHandlers<any> = {
   // Type '{ setup: () => Promise<void>; teardown: () => Promise<void>; }' 
   // is not assignable to type '(state: string, params?: { [name: string]: string; } | undefined) => Promise<any>'.
  'some state': { setup: async () => {}, teardown: async () => {} }, 
};