stop coercing falsy values to null before xhr.send() #11593
Conversation
| @@ -109,7 +109,12 @@ function createHttpBackend($browser, createXhr, $browserDefer, callbacks, rawDoc | |||
| } | |||
| } | |||
|
|
|||
| xhr.send(post || null); | |||
| if (post === false) { | |||
There was a problem hiding this comment.
Why special-case false only and not all falsy values ?
There was a problem hiding this comment.
That would be ideal. See #11552. Unfortunately Travis CI E2E tests failed in that case. If this fails, I'll dig deeper since it implies strongly that something is wrong with the tests. This is my first attempt at contributing to angular, so perhaps I don't understand the intended workflow well yet. I've been unable to build/test on my local machine per these instructions: https://docs.angularjs.org/misc/contribute.
There was a problem hiding this comment.
@johnhoffman, this build has failed because of a style error: else should be on the same line as the closing }:
// WRONG
}
else {
// RIGHT
} else {BTW, not all Travis failures means there is a problem with the code (you have to see the details; sometimes it's an unrelated timeout or something similar).
Additionally, you shouldn't create new PRs just to rerun the tests on Travis. Amending the latest commit and force-pushing to your repo, will update the PR and restart the Travis job.
Just so you know for future PRs :)
In any case, this is a legitimate failure, but I still wonder why only allow false ?
I've been unable to build/test on my local machine per these instructions
What was the problem ? You should be able to run the tests and it is always better to have them pass locally first, in order to avoid losing time (Travis takes much longer) and wasting resources.
There was a problem hiding this comment.
Thanks for your guidance, @gkalpak. I was able to fix my npm installation using https://github.com/brock/node-reinstall. I updated the code and new tests to reflect what I believe we should expect. Unit tests passed and E2E may be failing for extraneous reasons as you suggested. I'll update the PR per your instructions.
|
@johnhoffman, could you, please, squash all commits into one and add fix the commit message header and add Other than that it LGTM. |
|
Thanks, @gkalpak. Commits are squashed. Does the commit message look good? |
|
It LGTM. Thx @johnhoffman ! |
|
LGTM |
|
I'm of the opinion that we should also land this into 1.3 --- but lets let it bake for a bit first |
This is intended to resolve #11552.