Nock: Replace lodash with native where appropriate

[paulmelnikow]

I noticed there are a lot of uses of lodash. I think where lodash shines is in avoiding reimplementation of functions like mapValues within the codebase. However there are a lot of places in the codebase where native functions like Array#map, Object.assign, and Array.from(new Set(...)) would be equally understandable, and without the added layer of a library function.

A borderline case is _.flat which can be replaced by Array#reduce.

How are we feeling about code coverage? Is it important to nudge up the coverage before making changes like this in library code?

Related: This is still marked as supporting Node 4 in package.json. The tests run on 6+, and @gr2m I've heard you say you're just about ready to drop support for Node 6. Is there a roadmap or timeline in mind? I'm wondering if we can go right to object spread instead of using Object.assign.

[gr2m]

_.flat can often times be replace by [].concat :)

> [].concat(1, 2, [3])
< [1, 2, 3]

There will probably be lots of this all around the codebase, both in tests and the code base. It will be lots of fun to simplify the code base :)

This is still marked as supporting Node 4 in package.json. The tests run on 6+, and @gr2m I've heard you say you're just about ready to drop support for Node 6. Is there a roadmap or timeline in mind? I'm wondering if we can go right to object spread instead of using Object.assign.

Right now we only have https://medium.com/nodenock/the-nock-roadmap-where-we-are-and-where-we-are-going-8844df218649

How are we feeling about code coverage? Is it important to nudge up the coverage before making changes like this in library code?

Before we jump into refactorings, I’d like to try to have all parts of the code covered. 100% might be not worth it, but something close to it would be great.

---

But by all means, if you feel confident and enjoy to clean up the code a little, lets do it. We can drop Node 6 support today and start releasing preview features while still releasing updates to the current 10.x. It’s fairly easy not with semantic-release, thanks to https://github.com/semantic-release/semantic-release/issues/563

[paulmelnikow]

_.flat can often times be replace by [].concat :)

Nice!

We can drop Node 6 support today and start releasing preview features while still releasing updates to the current 10.x.

Could you explain that a bit further? I'm pretty new to semantic-release and can't say I've really digested the whole of what it can do.

[gr2m]

We use semantic-release to automate the release of new versions based on semantic commit messages. This works great if you only publish to @latest on npm, but it’s getting a little challenging if you have to release a patch version for a previous version or if you want to create preview versions such as [email protected], etc.

The new version of semantic-release makes that possible by simply following a few branch naming conventions. We can drop Node <8 support and continue work in a beta branch. We continue to use the commit conventions such as BREAKING CHANGE: drop Node 6 support, but it won’t release 11.0.0 from the beta branch, instead it will create beta preview versions and bump the last number with each release. Then when we are ready, we can merge beta into a next branch at which point semantic-release will release [email protected]@next. You’ll be able to install it with npm install nock@next and get the latest @next version`.

Then we merge next into master at which point semantic-release will simply point the @latest dist-tag on npm to the latest version, at which point everyone will get the new version.

While we work on this it’s well possible that someone runs into a bug with v10. To fix that, we create a new branch called v10.x and create pull requests against that. semantic-release will publish new releases from that branch, but only fix & feature releases, it won’t allow to publish breaking changes at all.

I know it’s quite a lot, but it's quite amazing.

[paulmelnikow]

Wow, that's really impressive! The issue was a bit thick to digest. Thanks for explaining it.

Sure, then why don't we go ahead and drop Node 6 support.

[gr2m]

I’ll get this going 👍

[lihail]

Before we jump into refactorings, I’d like to try to have all parts of the code covered. 100% might be not worth it, but something close to it would be great.

I agree, even though it's tempting to go and replace lodash with native implementation. What is the overall coverage today?

I'll get this going.

Don't you want to have a larger coverage first?..

[gr2m]

What is the overall coverage today?

We added a badge in the README, we are at 93%:

It links to Coveralls which has more details about the current coverage:
https://coveralls.io/github/nock/nock

You can also run npm test && npm run coverage to see the coverage report locally

[lihail]

@gr2m and about my other question?

[gr2m]

I have no strong feelings either way. I don’t mind replacing usage of lodash as long as it doesn’t make the code harder to read. The coverage is decent now thanks to the relentless effort by Paul :)

[jlilja]

Yeah thank you Paul! :)

While on the topic of code coverage. Would you say that ~ 80% would be enough? And that 80% would then cover the most essential pieces of code. I know that there is quite the debate on how much you must cover but I just want to know your thoughts :)

[lihail]

I see. Actually I was a bit afraid making this change without proper coverage first could cause trouble , but now that I saw the 93% coverage, sounds like we're good to go with this one.
I'm starting to take part in nock on the last couple of days and planning on doing so at least for a few more days, so I don't mind doing that too @gr2m, but it might take a few days until I have the time to reach it. So it's your call ;)

[gr2m]

Any contribution is welcome :)

@jlilja I’m a big fan of 100% and would like to reach it one. Once reached it becomes a test by itself. But we don’t need to reach it before we start refactoring, our current coverage is decent. I’d avoid to lower it through, each code change PR should slightly increase it

[lihail]

@jlila here's my opinion about tests: I can say that in my team (where I work), we honestly don't look at the coverage percentage. Instead, every new code that is inserted to the codebase must be inserted with tests too. The exception is the backend, in which we do only integration tests. I find this method a bit tedious but very rewarding in the long term, and I believe this project could benefit from it as well. We currently have 93% which is awesome, and we could decide that from now on every pull request will be approved only if it contains proper tests, regardless of the new test percentage.

[paulmelnikow]

I don't think 100% is a bad target for a library like this. There are a bajillion edge cases and lots of ways in which people's tests can be broken. If we're going to break things I'd rather do it deliberately and with notice in the changelog :) Agreed with @gr2m that we maybe shouldn't aim all the way for 100% before starting the refactor, as we may wand to just remove big chunks of code. @lihail the approach you're describing makes sense and that's for sure already the case here. However we're not talking about adding new code, but changing the guts of what's already here. Tests will let us do that with confidence.

Here's what I've been doing so far:

1. Rewriting tests using async and got, leaving behind comments when they can't easily be ported. I'm developing a new style and simplifying along the way.
2. Adding tests which cover little bits of functionality that don't have coverage now.
3. Removing legacy code (e.g. shims for old node versions).
4. Removing teeny bits of code here and there, which I'm highly confident is unreachable, and adding todo comments when it's not clear that it should be removed.

I can keep picking at it, though I think it would also be good to shake out tasks that other people can work on, and define a finish line.

One idea is to take a pass through the Coveralls result, maybe together on a video call, and decide what else needs to be fixed. Basically to decide specifically which pieces of the remaining coverage should be addressed as part of this "full test coverage" project.

[lihail]

@paulmelnikow great work so far! Maybe you could finish what you've already started and tell us what's the coverage at that point? Then we could decide what to do next.

[paulmelnikow]

Nothing in progress right now!

[lihail]

Oh lol, so what's the coverage now?

[paulmelnikow]

I think it would also be good to shake out tasks that other people can work on, and define a finish line.

One idea is to take a pass through the Coveralls result, maybe together on a video call, and decide what else needs to be fixed. Basically to decide specifically which pieces of the remaining coverage should be addressed as part of this "full test coverage" project.

@gr2m Any thoughts on that?

[gr2m]

I think that’s a great theme for the next nock open source Friday /cc @RichardLitt

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We try to do our best, but nock is maintained by volunteers and there is only so much we can do at a time. Thank you for your contributions.

[mastermatt]

@paulmelnikow should this be closed now?

[paulmelnikow]

I feel like we should replace the rest of the uses and drop the dependency. Since #1960 is a breaking change, we could merge the remaining lodash replacements at the same, time since our new object test might be slightly different.

[mastermatt]

@paulmelnikow I just opened https://github.com/nock/nock/pull/1963 which removes all, but one call to LoDash.

However, I think the last call is an issue.
https://github.com/nock/nock/blob/96e126cec4e9324b126f43e8971614d8db121763/lib/common.js#L566
_.set() is not a trivial function to replicate. ref 1, 2.

It seems the options are:

1. write/maintain our own version of that behemoth
2. keep LoDash
3. bring in another/smaller dep that only does JSON path expansion
4. drop the functionality that JSON path notation and nested objects are considered equal when comparing search parameters, JSON request bodies, and URL decoded request form bodies.

This seems like a silly reason to drop functionality, so I would not vote for the last option.

As for the other options, I question if we or any other smaller lib would be better suited for this utility than LoDash.
That being said, my personal position on LoDash is that it shouldn't be used for functionality natively in modern versions of Node, but it's in almost all of my projects because it provides great utilities not found in Node. I'm not sure what Nock gains by dropping it as a dependency.

[paulmelnikow]

Generally agreed, it's not worth dropping a dependency unless we want to drop this functionality. (Also, we could always depend on just lodash.set.)

It's a bit surprising to me that we accept the JSON path notation. It could lead to surprising failures if people are testing APIs with this kind of dotted key in it (where tests could pass unexpectedly).

It's also a little strange that we run it both on the spec and on the response. (It makes more sense to run it on the spec.)

It seems especially undesirable on query strings, where there isn't a standard way of dealing with nesting to begin with. (Didn't we remove some of that nesting functionality in v11?)

[mastermatt]

Didn't we remove some of that nesting functionality in v11?

We made it behave consistently.