Improve Header's forEach method compliance with browser implementation.#1074
Improve Header's forEach method compliance with browser implementation.#1074tekwiz merged 5 commits intonode-fetch:masterfrom lquixada:master
Conversation
src/headers.js
Outdated
| } | ||
|
|
||
| forEach(callback) { | ||
| forEach(callback, thisArg) { |
There was a problem hiding this comment.
| forEach(callback, thisArg) { | |
| forEach(callback, thisArg = undefined) { |
This allows headers.forEach.length === 1, which matches browsers.
There was a problem hiding this comment.
hmm a couple of things here:
- I can add that for extra clarity but the end result feels the same as
thisArgwill beundefinedwhenheaders.forEach.length === 1anyways. - To match browsers, the default value would be a global object such as window. On node, I guess
globalwould do the job. Try this:[1].forEach(function () { console.log(this === global) })
There was a problem hiding this comment.
I can add that for extra clarity
The result indeed is the same inside forEach, but the = undefined is essential to get the correct .length property. Try
console.log(({ f(a, b) {} }).f.length);
console.log(({ f(a, b = undefined) {} }).f.length);You will see that the first statement prints 2, while the second prints 1. We want the second behavior for forEach, and that's why you should add it. But indeed, the expected behavior is that if thisArg is not supplied, then it defaults to undefined.
By the way, the .length property of the forEach method is defined in the Web IDL standard, see "define the iteration methods" step 2.4.4.
To match browsers, the default value would be a global object such as window.
That's not quite right. In browsers, the default value is window only if we are not in strict mode:
Our test environments are all strict mode, but you can run Reflect.apply(function() { console.log(this); }, undefined, []) in the Node.js REPL (which is sloppy by default) to see that this is actually global in Node.js too.
All this is to say, you don't have to do anything special to get the global object behavior. (In fact, you cannot do anything here to replicate that in pure JavaScript, since there's no easy way of detecting whether a function is strict or sloppy.)
If you're interested in finding where this behavior is specified, it's in the ECMAScript standard, within OrdinaryCallBindThis abstract operation's steps 5–6.
There was a problem hiding this comment.
hey @TimothyGu ! Thanks for that! It was very informative. All suggestions are committed now. Let me know if there's anything else I should do.
Btw, there's just a lint issue going on that I'm not able to fix. :/
| const thisArg = {}; | ||
| headers.forEach(function () { | ||
| expect(this).to.equal(thisArg); | ||
| }, thisArg); |
There was a problem hiding this comment.
Can you also test the this value where thisArg is not supplied?
There was a problem hiding this comment.
sure! waiting for considerations on #1074 (comment).
|
See https://github.com/node-fetch/node-fetch/blob/master/src/request.js#L85. You could add a comment to ignore the check, like Line 53 in 4abbfd2 |
done! 👍 |
TimothyGu
left a comment
There was a problem hiding this comment.
LGTM. I think someone else has to review this though.

What is the purpose of this pull request?
What changes did you make? (provide an overview)
Added
thisArgto Header's forEach method andheaderparameter on its callback.Which issue (if any) does this pull request address?
#1072
Is there anything you'd like reviewers to know?
There's an unrelated lint issue on
src/request.jsthat's breaking the CI checks.