Make sure we return on errored browser requests.

[eiriksm]

Turns out that if browser requests are erroring out on an early stage (this could be error on DNS, cross-domain security cancellation or user has no internet connection), request will fail when trying the method caseless.has, because this will invoke Object.keys on undefined. So just wrapping the check here in a check for headers fixes the issue.

I also added a browesr test for this. The test proves the problem: it will fail if request.js is not updated (for example with this code).

I realize I could have also sent a PR to caseless to avoid calling Object keys on an undefined object, but seems to me that we should not even invoke caseless when we have no headers to work with.

Please let me know if you feel this should be solved in another way

Have a great week!

[simov]

Your fix is correct but I really think this should be fixed in caseless like:

this.dict = dict || {}

on line 2

[eiriksm]

OK, so fix both places, or just caseless?

I want to keep the test at least, but this will pass only when we update caseless in that case.

[simov]

IMO the fix should be applied only in caseless, then we keep the test in request of course.

Ping @nylen as he's the one that have publish right for these modules. Once this got published in caseless this PR can be fixed and merged.

My point about fixing this in caseless is that it's a bug in it in the first place.

[eiriksm]

Agreed. PR sent in request/caseless#17

[nylen]

👍 awesome! I actually ran into this issue the other day and didn't know what to make of it.

caseless 0.9.0 is out with that change.

I'd apply the fix here too (it's natural to say "do we have headers? if not, then don't do any of this stuff with headers"). But, as long as we fixed the issue and we have a test, that's the important part.

[nylen]

btw @eiriksm I've added you as a contributor. thanks for your work on browserified request, it is very much appreciated!

[eiriksm]

Added a commit with upgraded caseless to this PR. I don't really see the harm in having if (response.headers) in there still. In theory, it is a performance improvement ;)

And thanks for adding me as a contributor. Wow, I am honored :)

[nylen]

+1 from me, we fixed it twice.

I'll merge this tomorrow if no one objects or beats me to it.

[simov]

What would happen here and here if response.caseless is not being initialized on top?

It might be a very rare edge case but still you'll get cannot access whatever of undefined I'm for keeping the code without the if branch. Then if there isn't any headers the only check will be if (response.caseless.has('set-cookie')

[eiriksm]

I don't see how that would ever be possible. But be that as it may, you are of course right. The variable should not be uninitialized when it is being used later.

I just pushed an updated branch with the if removed.

Thanks for pointing that out, I should have checked for its usage before posting the suggestion.

[simov]

Looking good to me 👍