refactor: Replace several more uses of lodash with native, and related refactors

[paulmelnikow]

This renames Interceptor.matchAddress to Interceptor.matchOrigin which I think better fits what is being done in that method.

The terminology comes from e.g. URL.origin and what is sent for an Origin header.

Ref #1285

[mastermatt]

I'm a fan of this in general, but I am slightly concerned about that changes that are not equivalent.
isString covers strings created using the new statement, typeof contentEncoding === 'string' doesn't.
typeof value === 'object' returns true for null
typeof value === 'function' is false for async, generator, and proxy functions unlike isFunction.

Since we don't plan on removing LoDash as dependency, my concern is that those particular changes will hamper deving in the future.

[paulmelnikow]

Yea, this is worth discussing.

In a bit of searching, I haven't found any sources that argue for using String objects. Mostly people seem to discourage them, largely for this reason.

There are probably plenty of places in nock where String objects work, though there clearly aren't tests around them, and for sure they don't work in all cases. For example, we use === to check equality, which doesn't work on String objects:

> new String("gzip") === "gzip"
false

I guess the question is, do you think it's important to preserve some support for String objects?

In the interest of caution, I could see an argument that dropping these is a breaking change, though that really depends whether people actually use these.

We could also throw an error when given a String object, though then we'd still need an isString function whichi would sort of defeat the purpose of this cleanup.

Re async functions and generators, I haven't been able to reproduce the issues you mention:

> typeof (async () => {})
'function'
> typeof (new (class Foo { async thing() {} })().thing)
'function'
> typeof (async function*() {})
'function'

[mastermatt]

ah I didn't read the comment in LoDash's implementation of why/how then check for async/gens/etc. It has to do with a Safari quirk.
It also seems that since Node v7 someVar instanceof Function is also safe to use.

As for the string stuff, I personally consider use the String constructor a smell, but given it's large user base it could bite us.

[paulmelnikow]

It also seems that since Node v7 someVar instanceof Function is also safe to use.

We could do that instead. It seems a little nicer as Function is a global instead of a string literal.

As for the string stuff, I personally consider use the String constructor a smell, but given it's large user base it could bite us.

I suppose we could keep our own isString function and have it throw an error for using the String constructor. We could see if anyone complains (though honestly I think if anyone is using it, they're going to experience bugs in Nock related to that). In the long run we could phase out the check entirely.

[mastermatt]

I'm on the fence, but would probably vote for using typeof var === 'string' instead of rolling our own isString.

[paulmelnikow]

To be on the safe side we probably should change the string check in a breaking change.

[nockbot]

🎉 This PR is included in version 11.7.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[salmanm]

I'm facing a weird issue due to some changes in this PR. Specifically this. Here, the typeof callback is indeed "function", but callback instanceof Function is false. As bizzare as it sounds, it is true. See screenshot below

However, I do not have an explanation for this, so was wondering if any of you might have more insights on this? I tried debugging and put in a break point and verified this. The callback originates correctly but it gets messed up at this point. See screenshot below

I'm using node 10 (even tried 13).

I tried doing the same in the nock's tests but unfortunately I'm not able to reproduce it outside my codebase. I know it clearly implies that there must be something wrong with my setup, but no matter what the case is, why should an argument change from one step of the call stack to another!

Would appreciate any ideas/suggestions to try things.

[salmanm]

Looks like this could be the reason. jestjs/jest#2549.

And this seems like a good enough reason to go back to the typeof checking. Do you guys agree?

[paulmelnikow]

I'm fine with returning to typeof foo === 'function'. Ideally we should include a test that reproduces the behavior.

[salmanm]

Cool, here's the PR to change it back to typeof.

About tests, I'll try to put something together, but I'm not sure how much we'd cover if we just add the runInNewContext for some ops. I mean there are so many such instances and we don't know what could break in what scenario.

[paulmelnikow]

Either a single test of one instance would be fine, along with a comment explaining why this is necessary.

If that's too hard, we could skip the test and instead put the comment next to one of these checks.

[salmanm]

Ok tried something like this, but it still works. i.e. gives instanceof Function as true.

const vm = require('vm')
function fn () {}
console.log(vm.runInNewContext('[typeof fn, fn instanceof Function]', { fn }))

This is the case I was trying to reproduce, but not sure if that's easy enough.

I've added the comment as you suggested, please take a look at the PR.

[paulmelnikow]

This was resolved in #1850.

[nockbot]

🎉 This issue has been resolved in version 11.7.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀