For templates.find, return JSON parsed body

[aydrian]

User should not have to JSON.parse(res.body). It should be returned as JSON.

[coldacid]

This seems to go for all calls, not just templates.find.

[coldacid]

It seems to be a misuse of the json property of the options object passed to the request module's request function. The request body should be passed in as options.body and options.json should be true (not just truthy, apparently).

[aydrian]

You are correct and I have this fixed I the 2.0.0 wip because we determined it would be a breaking change for the 1.x line. We also determined it only happens for get requests.

[coldacid]

When will 2.0.0 be out? I've fixed it myself (along with adding a line missing from my PR for subaccounts; forgot to require it into the SparkPost class) but if it'll be a while longer then I'll have to run with my fork until 2.0.0 is actually released.

[aydrian]

We don't have a date yet. We are trying to make sure we cover any and all breaking changes. If you want to do a PR for what you left out of subaccounts, I can do a patch release.

[coldacid]

Already did.

[coldacid]

I'm not sure that the current code in the wip-2.0.0 branch is the best approach for this. Instead of the simple

  // set JSON (Always true)
  options.json = true;

I'd suggest checking that options.json isn't already set to a non-boolean value first. With the current state of the branch, no non-GET requests will succeed, because the body content (which is still being set on options.json everywhere) would be replaced with the value true.

My own fix attempt uses this code instead to deal with options.json, with the idea of a few lines in one place is easier to handle than many lines in almost as many files:

  // shift request body to options.body if set on options.json
  // otherwise default to accepting json responses
  if (options.json && typeof options.json !== 'boolean') {
    options.body = options.json;
    options.json = true;
  } else if (typeof options.json === 'undefined') {
    options.json = true;
  }

You might or might not find this a better approach, but it might help in moving things along towards final 2.0.0.

[aydrian]

@coldacid I think we found a better approach. If you'd like to take a look at PR #163. We also have tests checking that responses are returned as parsed json.

[aydrian]

Closed by #163

[coldacid]

Sorry I didn't get a chance earlier but yeah that looks just fine.