Add support for passing headers #119
Conversation
| return api.changeUserPassword({ auth: 'X', currentPassword: 'blabla', password: 'blabla2', invalidateTokens: true }) | ||
| .then((results) => { | ||
| results.should.eql({ | ||
| results.should.match({ |
There was a problem hiding this comment.
i'd rather be confident in exactly what we're sending in and should's .match() behavior strikes me as a bit too clever (docs). was .eql() causing issues?
There was a problem hiding this comment.
ah, i see - .match() is already being used in a bunch of places... 🤷♂️
| for (const key of Object.keys(headers)) { | ||
| req.set(key, headers[key]); | ||
| } | ||
| } |
There was a problem hiding this comment.
| } | ||
| if (query) { | ||
| req.query(query); | ||
| } |
There was a problem hiding this comment.
comparing to #115 which closely duplicates this functionality, i see a couple of key differences:
- i refactored all the base http methods (
.get(),.put(), etc) to accept an object instead of individual params as i find more than 3 fn args unwieldy - i tried to clean up
.createWebhook()as it has always accepted a headers object and thus conflicted with the newheadersarg / prop (see here) - i made
.downloadFile()useuriinstead ofurlfor uniformity
one downside of my approach is that it definitely represents a breaking change so a bump from 8 to 9 is warranted.
i'm on the fence... i'm not sure how disruptive it'd be to go with my refactor as the appeal of the lib isn't its base http methods... but if i had to pick i guess i'd go with my approach if only because of item 2 in my list above. not a super-strongly held position though.
There was a problem hiding this comment.
actually, there's another even bigger difference between this and #115 - the later extends every high-level method (e.g. .sendOtp(), .createCustomer(), etc) to optionally accept headers which is think is useful (perhaps necessary?)
so, eesh... yeah, another point in favor of adopting #115 i guess. sorry for the churn, let's chat about how best to divide an conquer 👍
|
@suda ok, per #119 (comment) i went ahead and rebased #115 and ported your changes over to another PR (#121 ) which currently targets the if all that looks good, i say we ship it (first merge #115 branch to sorry again for the churn / weirdness on all this 🙏❤️ |
No description provided.