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

Remove need to duplicate VerifierOptions #377

Merged
merged 3 commits into from
Oct 17, 2019

Conversation

mefellows
Copy link
Member

Removes duplicated VerifierOptions fixed in pact-foundation/pact-js-core#178.

…echnically a breaking change 'pactBrokerBaseUrl' is a deprecated and useless field. Need to consider what we do here when we release
@mefellows mefellows force-pushed the feat/remove-duplicated-pact-node-options branch from 5ab43f1 to 19967dc Compare October 10, 2019 02:13
@TimothyJones
Copy link
Contributor

From your commit message:

technically a breaking change 'pactBrokerBaseUrl' is a deprecated and useless field. Need to consider what we do here when we release

I think we probably should update all the types in one release and bump the major version - rather than just verifier options.

I'm not very typescript savvy, but I think removing all the repeated types means anyone using the types will need to make code changes?

It would be nice if we could warn users about the deprecation, but I'm also ok with a release notes that say what the new field is called

(as an aside, I think there's some inconsistency around the naming of broker fields in pact-node- maybe we could/should clean that up at the same time?)

@coveralls
Copy link

coveralls commented Oct 10, 2019

Coverage Status

Coverage remained the same at 94.758% when pulling 6ef5394 on feat/remove-duplicated-pact-node-options into d2c5453 on master.

@vgrigoruk
Copy link
Contributor

Could we also bump pact-node to 9.0.6 here, so we get pact-foundation/pact-js-core#189 ?

@vgrigoruk
Copy link
Contributor

@TimothyJones @mefellows can I help somehow to get this merged?

@mefellows
Copy link
Member Author

I'm not very typescript savvy, but I think removing all the repeated types means anyone using the types will need to make code changes?

Yes, if they were using the old type - it would have to be a major version bump (even though most would use the implicit type required here - as you can see, I didn't need to change any of the regression Typescript test cases but they still work).

It would be nice if we could warn users about the deprecation, but I'm also ok with a release notes that say what the new field is called

👍

(as an aside, I think there's some inconsistency around the naming of broker fields in pact-node- maybe we could/should clean that up at the same time?)

There is, but I think we should defer that re-organisation when we move to the new Rust binary because that will likely be a significant change to the API.

In order to keep things moving I think I'll just merge this in for now.

@mefellows mefellows merged commit ced7188 into master Oct 17, 2019
@mefellows mefellows deleted the feat/remove-duplicated-pact-node-options branch October 17, 2019 21:33
@mefellows
Copy link
Member Author

@vgrigoruk I put out a new release this morning, with the upgraded pact-node dependency. Thanks for your patience.

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.

None yet

4 participants