Replace lodash with native where appropriate #1285

paulmelnikow · 2018-12-17T00:48:59Z

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.

The text was updated successfully, but these errors were encountered:

gr2m · 2018-12-17T01:02:42Z

_.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 semantic-release/semantic-release#563

paulmelnikow · 2018-12-17T01:15:11Z

_.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 · 2018-12-17T01:26:06Z

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 nock@11.0.0-beta.1, 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 nock@11.0.0@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 · 2018-12-17T02:28:06Z

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 · 2018-12-17T02:50:47Z

I’ll get this going 👍

lihail · 2019-01-07T06:26:06Z

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 · 2019-01-07T16:33:24Z

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 · 2019-01-07T16:36:33Z

@gr2m and about my other question?

gr2m · 2019-01-07T16:44:52Z

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 · 2019-01-07T16:50:28Z

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 · 2019-01-07T16:51:47Z

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 · 2019-01-07T16:59:41Z

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 · 2019-01-07T16:59:47Z

@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 · 2019-01-07T17:06:19Z

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:

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.
Adding tests which cover little bits of functionality that don't have coverage now.
Removing legacy code (e.g. shims for old node versions).
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 · 2019-01-07T17:30:39Z

@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 · 2019-01-07T17:32:05Z

Nothing in progress right now!

lihail · 2019-01-07T17:44:31Z

Oh lol, so what's the coverage now?

paulmelnikow · 2019-01-08T19:43:35Z

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 · 2019-01-08T20:40:02Z

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

stale · 2019-05-16T15:57:44Z

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.

For: #1285 Not logic changes, just making use of newer JS.

* refactor(common): replace lodash.forOwn with Array.reduce This commit replaces usage of lodash.forOwn with a combination of Object.keys() & Array.reduce. Related #1285 * refactor(commong): use Array.map This commit simplifies array formatting by using Array.map instead of a plain for loop. * test(common): add unit test coverage for #formatQueryValue This commit adds unit test coverage for #formatQueryValue. Related #1404 * chore: run prettier to fix lint errors * fix: use Object.entries() in favor of Object.keys() This commit replaces the usage of Object.keys() with Object.entries() to address [this](#1598 (comment)) comment. * test(common): break down tests This commit addresses the following comments: 1- #1598 (comment) 2- #1598 (comment) * chore: run prettier to fix lint errors * refactor(common): replace lodash.forOwn with Array.reduce This commit replaces usage of lodash.forOwn with a combination of Object.keys() & Array.reduce. Related #1285 * refactor(commong): use Array.map This commit simplifies array formatting by using Array.map instead of a plain for loop. * test(common): add unit test coverage for #formatQueryValue This commit adds unit test coverage for #formatQueryValue. Related #1404 * chore: run prettier to fix lint errors * fix: use Object.entries() in favor of Object.keys() This commit replaces the usage of Object.keys() with Object.entries() to address [this](#1598 (comment)) comment. * test(common): break down tests This commit addresses the following comments: 1- #1598 (comment) 2- #1598 (comment) * chore: run prettier to fix lint errors * chore: remove unnecessary formatting changes

* chore: update some docstrs * refactor(recorder): clean up default options using spread and deconstructing syntax * refactor: remove unnecessary lodash usage For #1285 * refactor(interceptor): remove dead code

For: #1285 Not logic changes, just making use of newer JS.

* refactor(common): replace lodash.forOwn with Array.reduce This commit replaces usage of lodash.forOwn with a combination of Object.keys() & Array.reduce. Related #1285 * refactor(commong): use Array.map This commit simplifies array formatting by using Array.map instead of a plain for loop. * test(common): add unit test coverage for #formatQueryValue This commit adds unit test coverage for #formatQueryValue. Related #1404 * chore: run prettier to fix lint errors * fix: use Object.entries() in favor of Object.keys() This commit replaces the usage of Object.keys() with Object.entries() to address [this](#1598 (comment)) comment. * test(common): break down tests This commit addresses the following comments: 1- #1598 (comment) 2- #1598 (comment) * chore: run prettier to fix lint errors * refactor(common): replace lodash.forOwn with Array.reduce This commit replaces usage of lodash.forOwn with a combination of Object.keys() & Array.reduce. Related #1285 * refactor(commong): use Array.map This commit simplifies array formatting by using Array.map instead of a plain for loop. * test(common): add unit test coverage for #formatQueryValue This commit adds unit test coverage for #formatQueryValue. Related #1404 * chore: run prettier to fix lint errors * fix: use Object.entries() in favor of Object.keys() This commit replaces the usage of Object.keys() with Object.entries() to address [this](#1598 (comment)) comment. * test(common): break down tests This commit addresses the following comments: 1- #1598 (comment) 2- #1598 (comment) * chore: run prettier to fix lint errors * chore: remove unnecessary formatting changes

* chore: update some docstrs * refactor(recorder): clean up default options using spread and deconstructing syntax * refactor: remove unnecessary lodash usage For #1285 * refactor(interceptor): remove dead code

For: #1285 Not logic changes, just making use of newer JS.

* refactor(common): replace lodash.forOwn with Array.reduce This commit replaces usage of lodash.forOwn with a combination of Object.keys() & Array.reduce. Related #1285 * refactor(commong): use Array.map This commit simplifies array formatting by using Array.map instead of a plain for loop. * test(common): add unit test coverage for #formatQueryValue This commit adds unit test coverage for #formatQueryValue. Related #1404 * chore: run prettier to fix lint errors * fix: use Object.entries() in favor of Object.keys() This commit replaces the usage of Object.keys() with Object.entries() to address [this](#1598 (comment)) comment. * test(common): break down tests This commit addresses the following comments: 1- #1598 (comment) 2- #1598 (comment) * chore: run prettier to fix lint errors * refactor(common): replace lodash.forOwn with Array.reduce This commit replaces usage of lodash.forOwn with a combination of Object.keys() & Array.reduce. Related #1285 * refactor(commong): use Array.map This commit simplifies array formatting by using Array.map instead of a plain for loop. * test(common): add unit test coverage for #formatQueryValue This commit adds unit test coverage for #formatQueryValue. Related #1404 * chore: run prettier to fix lint errors * fix: use Object.entries() in favor of Object.keys() This commit replaces the usage of Object.keys() with Object.entries() to address [this](#1598 (comment)) comment. * test(common): break down tests This commit addresses the following comments: 1- #1598 (comment) 2- #1598 (comment) * chore: run prettier to fix lint errors * chore: remove unnecessary formatting changes

* chore: update some docstrs * refactor(recorder): clean up default options using spread and deconstructing syntax * refactor: remove unnecessary lodash usage For #1285 * refactor(interceptor): remove dead code

For: #1285 Not logic changes, just making use of newer JS.

…d refactors (#1754) 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](https://developer.mozilla.org/en-US/docs/Web/API/URL/origin) and what is sent for an [Origin header](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Origin). In Nock v12 we will stop checking for strings using the String constructor (i.e. `new String`). Ref #1285

Ref #1285

mastermatt · 2020-03-28T18:22:13Z

@paulmelnikow should this be closed now?

paulmelnikow · 2020-03-28T20:03:09Z

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.

- `isMap` was added to `util.types` in Node 10.0 https://nodejs.org/api/util.html#util_util_types_ismap_value - `isPlainObject` and `mapValue` both came from LoDash's master branch, which is the pending v5. Ref: nock#1285

mastermatt · 2020-03-28T23:02:38Z

@paulmelnikow I just opened #1963 which removes all, but one call to LoDash.

However, I think the last call is an issue.

nock/lib/common.js

Line 566 in 96e126c

Object.entries(input).reduce((acc, [k, v]) => _.set(acc, k, v), {})

_.set() is not a trivial function to replicate. ref 1, 2.

It seems the options are:

write/maintain our own version of that behemoth
keep LoDash
bring in another/smaller dep that only does JSON path expansion
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 · 2020-03-29T00:13:07Z

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 · 2020-03-29T14:56:46Z

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

We made it behave consistently.

- `isMap` was added to `util.types` in Node 10.0. https://nodejs.org/api/util.html#util_util_types_ismap_value - `isPlainObject` and `mapValue` both came from LoDash's master branch, which is the pending v5. - Replace LoDash dependency with `lodash.set` to cover the one use we still have. Ref: nock#1285

- `isMap` was added to `util.types` in Node 10.0. https://nodejs.org/api/util.html#util_util_types_ismap_value - `isPlainObject` and `mapValue` both came from LoDash's master branch, which is the pending v5. - Replace LoDash dependency with `lodash.set` to cover the one use we still have. Ref: #1285

gr2m mentioned this issue Dec 17, 2018

bump semantic-release to v16 beta #1293

Merged

paulmelnikow mentioned this issue Dec 17, 2018

test(test_basic_auth): Rewrite using got + async/await #1294

Merged

gr2m mentioned this issue Dec 17, 2018

build on branches for semantic-release preview/next/maintenance releases #1297

Merged

weekly-digest bot mentioned this issue Dec 23, 2018

Weekly Digest (16 December, 2018 - 23 December, 2018) #1331

Closed

paulmelnikow mentioned this issue Jan 11, 2019

Next Maintainathon: Friday, January 18th! #1374

Closed

RichardLitt added the pull request welcome label Feb 15, 2019

paulmelnikow mentioned this issue May 6, 2019

Update/clarify interceptor.reply param juggling. #1520

Merged

stale bot added the stale label May 16, 2019

paulmelnikow added the pinned label May 16, 2019

stale bot removed the stale label May 16, 2019

paulmelnikow pushed a commit that referenced this issue Aug 22, 2019

refactor: replace some lodash usage with native (#1685)

89eb03a

For: #1285 Not logic changes, just making use of newer JS.

weekly-digest bot mentioned this issue Aug 25, 2019

Weekly Digest (18 August, 2019 - 25 August, 2019) #1697

Closed

gr2m pushed a commit that referenced this issue Sep 4, 2019

refactor: replace some lodash usage with native (#1685)

afcbee2

For: #1285 Not logic changes, just making use of newer JS.

gr2m pushed a commit that referenced this issue Sep 4, 2019

refactor: replace some lodash usage with native (#1685)

c0372a2

For: #1285 Not logic changes, just making use of newer JS.

gr2m pushed a commit that referenced this issue Sep 5, 2019

refactor: replace some lodash usage with native (#1685)

4ff3c66

For: #1285 Not logic changes, just making use of newer JS.

paulmelnikow mentioned this issue Oct 19, 2019

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

Merged

paulmelnikow added a commit that referenced this issue Feb 18, 2020

Replace a few more uses of lodash

c8a1bc2

Ref #1285

paulmelnikow mentioned this issue Feb 18, 2020

fix: Replace a few more uses of lodash #1916

Merged

paulmelnikow added a commit that referenced this issue Feb 19, 2020

fix: Replace a few more uses of lodash (#1916)

1f47f1a

Ref #1285

snyk-bot mentioned this issue Mar 13, 2020

[Snyk] Upgrade nock from 12.0.0 to 12.0.1 salty-pig/swapi-node#68

Merged

This was referenced Mar 24, 2020

[Snyk] Upgrade nock from 12.0.1 to 12.0.2 salty-pig/swapi-node#72

Merged

[Snyk] Upgrade nock from 12.0.0 to 12.0.2 nodeshift/nodeshift#427

Merged

[Snyk] Upgrade nock from 12.0.0 to 12.0.2 nodeshift/openshift-rest-client#201

Merged

mastermatt mentioned this issue Mar 28, 2020

refactor: replace LoDash isPlainObject, mapValues, and isMap #1963

Merged

mastermatt closed this as completed Aug 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace lodash with native where appropriate #1285

Replace lodash with native where appropriate #1285

paulmelnikow commented Dec 17, 2018

gr2m commented Dec 17, 2018

paulmelnikow commented Dec 17, 2018

gr2m commented Dec 17, 2018

paulmelnikow commented Dec 17, 2018 •

edited

gr2m commented Dec 17, 2018

lihail commented Jan 7, 2019 •

edited

gr2m commented Jan 7, 2019

lihail commented Jan 7, 2019 •

edited

gr2m commented Jan 7, 2019

jlilja commented Jan 7, 2019

lihail commented Jan 7, 2019

gr2m commented Jan 7, 2019

lihail commented Jan 7, 2019

paulmelnikow commented Jan 7, 2019

lihail commented Jan 7, 2019

paulmelnikow commented Jan 7, 2019

lihail commented Jan 7, 2019

paulmelnikow commented Jan 8, 2019

gr2m commented Jan 8, 2019

stale bot commented May 16, 2019

mastermatt commented Mar 28, 2020

paulmelnikow commented Mar 28, 2020

mastermatt commented Mar 28, 2020

paulmelnikow commented Mar 29, 2020

mastermatt commented Mar 29, 2020

Replace lodash with native where appropriate #1285

Replace lodash with native where appropriate #1285

Comments

paulmelnikow commented Dec 17, 2018

gr2m commented Dec 17, 2018

paulmelnikow commented Dec 17, 2018

gr2m commented Dec 17, 2018

paulmelnikow commented Dec 17, 2018 • edited

gr2m commented Dec 17, 2018

lihail commented Jan 7, 2019 • edited

gr2m commented Jan 7, 2019

lihail commented Jan 7, 2019 • edited

gr2m commented Jan 7, 2019

jlilja commented Jan 7, 2019

lihail commented Jan 7, 2019

gr2m commented Jan 7, 2019

lihail commented Jan 7, 2019

paulmelnikow commented Jan 7, 2019

lihail commented Jan 7, 2019

paulmelnikow commented Jan 7, 2019

lihail commented Jan 7, 2019

paulmelnikow commented Jan 8, 2019

gr2m commented Jan 8, 2019

stale bot commented May 16, 2019

mastermatt commented Mar 28, 2020

paulmelnikow commented Mar 28, 2020

mastermatt commented Mar 28, 2020

paulmelnikow commented Mar 29, 2020

mastermatt commented Mar 29, 2020

paulmelnikow commented Dec 17, 2018 •

edited

lihail commented Jan 7, 2019 •

edited

lihail commented Jan 7, 2019 •

edited