Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: stateHandlers type in VerifierOptions #1120

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ben-styling
Copy link

@ben-styling ben-styling commented Oct 4, 2023

  • npm run dist works locally (this will run tests, lint and build)
  • Commit messages are ready to go in the changelog (see below for details)
  • PR template filled in (see below for details)

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!

@ben-styling ben-styling marked this pull request as draft October 4, 2023 13:33
@ben-styling ben-styling marked this pull request as ready for review October 4, 2023 14:21
@YOU54F
Copy link
Member

YOU54F commented Oct 17, 2023

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
Copy link
Contributor

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 #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 () => {} }, 
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants