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

Solves #920 #921

Merged
merged 1 commit into from
Sep 23, 2017
Merged

Solves #920 #921

merged 1 commit into from
Sep 23, 2017

Conversation

edorivai
Copy link
Contributor

Straightforward fix of #920

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 92.702% when pulling a109522 on edorivai:master into 17d7c25 on node-nock:master.

@gr2m gr2m merged commit 635fbf9 into nock:master Sep 23, 2017
@gr2m
Copy link
Member

gr2m commented Sep 23, 2017

This change introduced a bug that surfaced at hoodiehq/hoodie-store-server-api#21, I’m investigating

@gr2m
Copy link
Member

gr2m commented Sep 23, 2017

Actually the specs are wrong and your fix showed that for the first time 👏

gr2m added a commit to hoodiehq/hoodie-store-server-api that referenced this pull request Sep 23, 2017
gr2m added a commit to hoodiehq/hoodie-store-server-api that referenced this pull request Sep 23, 2017
@paulmelnikow
Copy link
Member

What what?! 👏 Thanks @edorivai!

@gr2m gr2m mentioned this pull request Sep 25, 2017
cjwatson added a commit to cjwatson/build.snapcraft.io that referenced this pull request Sep 26, 2017
nock 9.0.19 started enforcing the specified request body
(nock/nock#921), so tighten up our tests to
cope with that.
cjwatson added a commit to canonical-web-and-design/build.snapcraft.io that referenced this pull request Sep 26, 2017
nock 9.0.19 started enforcing the specified request body
(nock/nock#921), so tighten up our tests to
cope with that.
@CodeMan99
Copy link

Why does this change not count as a breaking change? This broke my tests and I had to dig to this pull request to find out why.

@paulmelnikow
Copy link
Member

I have experience with some linters (e.g. jscs) which took an extremely strict interpretation of semver which said "a new error is a breaking change". However, for most libraries, fixing behavior that was previously outright incorrect is not usually a breaking change.

Since this change seems to have a high ecosystem impact, perhaps we should consider reverting it and re-shipping it with a major version change.

Alternatively we could make sure the intended behavior is clear in the changelog. I agree that developers shouldn't have to come to the PR.

@gr2m
Copy link
Member

gr2m commented Sep 28, 2017

@CodeMan99 welcome to open source, it never is perfect, but we are all in this together :) we are all volunteering our time here, please keep that in mind 🤗

@CodeMan99
Copy link

CodeMan99 commented Sep 28, 2017

@gr2m Was just an honest question, wasn't trying to be firm or abrasive. Sorry.

@gr2m
Copy link
Member

gr2m commented Sep 28, 2017

No worries :) I understand your frustration, it caused the same for several of my projects and caused some work. We’ll soon automate releases with better changelogs, that should help a little.

@focusaurus
Copy link

A few tips if you unexpectedly hit this and don't know what to do.

  1. See the full body so you can find the extra properties that are being sent but are missing from your expectation body.
nock(host).post("/", (body) => {
  console.log("Actual body: ", JSON.stringify(body, null, 2)); // fixme
  return true;
})
  1. If you want loose matching, try integrating tmatch.
const tmatch = require("tmatch");

nock(host).post("/", (body) => tmatch(body, {key1: "foo", key2: /^z/}))

If you use the pattern above a lot, you may want to factor out a helper function.

@lock
Copy link

lock bot commented Sep 13, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you!

@lock lock bot locked as resolved and limited conversation to collaborators Sep 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants