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

Allow optionally setting API call Headers #26

Closed
wants to merge 8 commits into from

Conversation

UnbounDev
Copy link

thelinmichael's work to provide access to response headers in PR #25 is awesome, this PR completes the loop by allowing users to set the Header values when making API calls.

```javascript
spotifyApi.getPlaylist('odesza','0uxUkSQHZVM7so7msXl9Ck', { headers: { 'If-None-Match': 'AAAANPBn03qInch471K02Rncz5IHWVja'} })
.then(function(data) {
if(data.statusCode !== 304)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor formatting comment, but if and parentheses should have a space between them according to Code Conventions for the JavaScript Programming Language.

@thelinmichael
Copy link
Owner

Thanks a lot for this pull request @UnbounDev! This change makes total sense and should've been included in the 2.0.0 version.

A couple of thoughts:

  • Always recommended to back up changes with tests to make sure they don't break in the future. Also, it's great for documentation purposes for other developers.
  • Always make changes to a feature branch, it'll make life easier if changes have been made concurrently to the main repository's master branch. (See Github Flow) (No need to make a different branch for this specific PR though.)
  • I think it's okay to send the headers through the options variable since they're indeed optional. I can imagine that some developers may find it a bit confusing to mix query- or body-parameters with headers, so we'd need to add a couple of more examples using it. I haven't anything new in the /examples folder for a while, so I'll look at that.

@JMPerez, any usability concerns? E.g.

{
   "headers" : {
     "If-None-Match" : "cachedEtagValue"
   },
   "limit" : 10,
   "offset" : 15
}

Other options to this would be to split the options object parameters and a headers, or to add another argument to the method, e.g. addTracksToPlaylist(userId, playlistId, tracks, options, headerOptions, callback). Both of these feels a bit overkill.

@JMPerez
Copy link
Collaborator

JMPerez commented Mar 17, 2015

@thelinmichael Some wrappers use an object with a key for the data, and another key for headers, like jQuery.ajax with data and headers:

$.ajax({
  ...
  headers: {
    "If-None-Match" : "cachedEtagValue"
  },
  data: {
    "limit" : 10,
    "offset" : 15
  }
})

That would keep the data separated from the headers and would prevent collisions. Although very unlikely, we might have a headers query parameter at some point. However, this approach is not backwards-compatible and forces every call to add that data key.

What I wouldn't like to see is having both options and headerOptions in the method's signature. I don't see the point of splitting them as 2 parameters.

@UnbounDev
Copy link
Author

@thelinmichael apologies, your're correct I ought to have followed gitflow (it was a late night).

@JMPerez thelinmichael makes a great point, I'd prefer to redo this work under a feature branch (if only to maintain an appropriate version sequence - this PR should only be v2.1.0 not v2.1.1, again, long night!). I'd suggest using the first format to maintain a non-breaking change:

{
   "headers" : {
     "If-None-Match" : "cachedEtagValue"
   },
   "limit" : 10,
   "offset" : 15
}

If at a later date there is a need for a query param header we can introduce a breaking change and switch to the { "headers": {}, "data": {} } convention.

@JMPerez
Copy link
Collaborator

JMPerez commented Mar 2, 2016

Reopening this PR in #47

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

3 participants