Fix xhr.assertSuccess promise

[mkhatib]

The else part is obvious. What might not be obvious is the test change. @dvoytenko I dropped (changed actually) the test of the previous return a resolved promise issue inside the constructor of the promise, this doesn't actually to have the effect that we thought it would. Tried it with a snippet like the following:

function myfunc() { 
  return new Promise((resolve, reject)  => {
    return Promise.resolve();
  });
}

myfunc().then(() => console.log('I will never be resolved so I will not log...'));

I am not sure if I am still missing the issue with the previous return statement - if I am can you write a small snippet (like the above) that could show the problem? Probably will help me understand it better.

[dvoytenko]

LGTM to the PR - this is definitely a must fix thing (may need to interrupt canary to pick it up). Find me to discuss the question offline.

[jridgewell]

Isn't the issue that you're not using .should.eventually.be.rejected? Or, you could return the promise with a flip-flop:

return assertSuccess(createResponseInstance('{"a": 2}', mockXhr)).then(() => {
  throw new Error('should never reach this'); // This makes a resolved promise blow up the tests
}, (err) => {
  return; // this makes the rejected promise pass the tests
});

Importantly, this won't hide a never resolving promise.

[mkhatib]

I've tested with eventually.be.rejected that wasn't the issue. The initial issue we thought might happen (on a previous PR #3669) is that returning the already resolved promise returned from response.json() inside the new Promise constructor would actually resolve the promise instead of actually the intended behavior of rejecting it. This doesn't see true that's why this test no longer needed/valid.

[jridgewell]

I'm not seeing a return? We did immediately resolve when there was a JSON error response, which we definitely should have caught.

[mkhatib]

Yeah that was previously, we added that test to catch that case and then removed the return statement.

We did immediately resolve when there was a JSON error response, which we definitely should have caught.

Yes the edit to this test makes sure we catch that.

[jridgewell]

PR won't let me comment on the correct line. On L361, there's no need to create another promise instance (chaining a #catch), just pass the onRejected as the second param of the #then:

response.json().then(json => {
  err.responseJson = json;
  reject(err);
}, () => {
  reject(err);
});

[mkhatib]

Didn't realize chaining .catch creates another instance of a promise. Is passing onRejected just more efficient to do than chaining then?

[jridgewell]

Is passing onRejected just more efficient to do than chaining then?

It avoids another Promise instance and runs in the same tick.

[mkhatib]

Done.

[jridgewell]

LGTM.