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

Add cors flag in PactOptions #187

Closed
benquinteros opened this issue Apr 30, 2020 · 5 comments
Closed

Add cors flag in PactOptions #187

benquinteros opened this issue Apr 30, 2020 · 5 comments

Comments

@benquinteros
Copy link

Screen Shot 2020-04-30 at 11 09 13 am

Are we able to add the cors flag like so, would save having to manually mock a cors interaction.

@mefellows
Copy link
Member

I'm wondering if it makes sense to have PactOptions duplicated in this package - could it not just reference the upstream PactOptions interface directly? This would mean it's always up-to-date with pact?

@mefellows
Copy link
Member

It makes sense to me @benquinteros, but i'll let Tim/Yousaf comment as they are the maintainers of this repo.

@YOU54F
Copy link
Member

YOU54F commented Apr 30, 2020

agree with @mefellows, it is better to reference the interface from pact-js directly. Happy for a PR proposal 👍 otherwise I look to address in the next couple of days

@TimothyJones
Copy link
Contributor

I remembered why we had a new type - it's because we add timeout to the options. But, you're right that it jest-pact should use the upstream PactOptions directly.

I've fixed this in #188 , which

  • Removes the definition of PactOptions
  • Introduces JestPactOptions, which unions the upstream PactOptions object and includes the timeout

I wanted to hold off merging for a bit of discussion - simple questions, but worth thinking about before we release a version 1:

  • Should JestPactOptions be called PactOptions? I'm not sure. I thought about separating out the Jest-specific options, but I'm not a big fan of having two ways of specifying options - users of this code shouldn't have to know what part of the framework they're configuring
  • For extra settings, should we put them under a namespace? jestTimeout? jest.Timeout?

It's worth keeping in mind that currently this is really also jasmine-pact as well as jest-pact, since it exclusively uses jasmine features.

@TimothyJones
Copy link
Contributor

I forgot to close this issue before - this is fixed in 0.5.3.

It's technically a breaking change if you've explicitly used the type PactOptions, because I've renamed it to JestPactOptions - however, most people probably use it implicitly.

Since this is getting more traction, we'll release a version 1.0.0 soon, which will let us be clearer about breaking changes.

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

No branches or pull requests

4 participants