Remove lodash.noop lib dependency

[SkeLLLa]

Lodash noop lib is useless and it's better to use just:

var noop = Function;

Because of following reasons:

• It's shorter
• It's faster
• It doesn't take space in node_modules
• It will not fail in case of deletion of lodash.noop
• It will not take space in package.json and yarn.lock
• Even lodash devs removed lodash.noop from their code.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[SkeLLLa]

I signed it!

[googlebot]

CLAs look good, thanks!

[coveralls]

Coverage remained the same at 94.233% when pulling 0d4df2c96716b16741852c1411fa37721271fae1 on SkeLLLa:feature/remove-useless-lib into 642fba8 on google:master.

[murgatroid99]

Why Function instead of function() {}? Strictly speaking, Function isn't a noop; It's a function that returns a new function based on the input.

[jmuk]

noop is not equivalent to Function itself. The latter is a constructor of a function. The equivalent to noop is function() {}

[jmdobry]

Thanks!

[jmdobry]

From a noop perspective, the following should all work:

var noop = function () {};
var noop = new Function();
var noop = Function();
var noop = Function;
var noop = Function.prototype;

Correction: These would all work fine if you were to pass them to say, setTimeout. If values will be passed to them, var noop = Function; should be dropped.

[jmdobry]

After taking a closer look at how noop is used in the code (it's possible it might receive an Error instance), I think you should switch to one of the following:

var noop = function () {};
var noop = Function();
var noop = new Function();
var noop = Function.prototype;

[cristiancavalli]

Why not create and use a named, static noOp in utils.js and just import it? In the current form you would be needlessly creating a new instance of the same function signature in each file and allocating 4 unnecessary function instances.

[jmdobry]

With var noop = Function.prototype; we can avoid 4 extra function instances and avoid modifying utils.js.

Ultimately though, I don't think this lib should be using noop at all, the callback should be mandatory (breaking change) where the lib is currently falling back to the noop.

[SkeLLLa]

@cristiancavalli usually in my libs I prefer to set once global.noop = Function. In case of public lib it can do some harm to others, so it's probably not an option. But in any case creating 1 or 4 anonymous functions will not have significant affect on performance.

Also I think var noop = Function.prototype will be better, because it will not create additional instances, just like with var noop = Function.

So if var noop = Function.prototype seems good for all, then i'll change to it.

[cristiancavalli]

@jmdobry What is the problem making noOp in utils? It's a util right?
@SkeLLLa Thanks, but the performance impact is not the point -- it's the idea that each top-level has a different localized assignment and implementation for what is ostensibly a globally-shared entity thereby creating an anti-pattern

[SkeLLLa]

@cristiancavalli as I've wrote using global.noop is not an option for a public library. Also using lodash.noop as 3rd party module is also kind of antipattern.

As for implementing in some module a simple noop function will require to add require('utils.js') in all files where you need noop. Even if not utils are required in all files that not a guarantee that it will be the same in future. So I would agree with @jmdobry that's var noop = Function.prototype; will be good enough.

[cristiancavalli]

also @jmdobry Every single file in this PR already requires util.js except for jwtaccess.js. Why would we create a localized implementation in each file of a function which is a util and only needs to be implemented once.

[cristiancavalli]

@SkeLLLa All the files (save 1) in this PR already requires util

[cristiancavalli]

Also @SkeLLLa not sure how using a third-party module is an anti-pattern? Even for something like noOp. Whereas this copy-paste solution seems like a classic case of abstraction inversion to me on the class level

[SkeLLLa]

@cristiancavalli as for me it's definite antipattern to require module that doesn't contain any logic and have only one line of code that could be easilly replaced by one line in simple javascript code. It's like adding two numbers in js with help of jquery

[cristiancavalli]

@SkeLLLa I understand your complaint - I've had it many times myself and I think subbing out lodash.noOp is better than keeping it in but I don't what you're citing is an anti-pattern. An inefficiency? Totally. Violating any rules of abstraction/inheritance/relation? I can't see any, but I may not be seeing it

[jmdobry]

I'm fine with either option in this case, but @cristiancavalli makes a good point that the files are already importing utils.js anyway, so I'm cool with moving the noop definition to utils.js.

[coveralls]

Coverage remained the same at 94.233% when pulling 415cad7 on SkeLLLa:feature/remove-useless-lib into 642fba8 on google:master.

[SkeLLLa]

@jmdobry file https://github.com/google/google-auth-library-nodejs/blob/master/lib/auth/jwtaccess.js isn't importing utils

[cristiancavalli]

@SkeLLLa Just by LOC, even with the change to jwtaccess.js for utils, you'd still come in under the current change footprint. (just sayin')

[jmdobry]

Alright folks, just took a closer look at the code, and utils.js is not even used by the library, it's only used in one place in the tests. Turns out we confused var util = require('util'); with var utils = require('../utils');. So, to move the noop to utils.js is now a "bigger" change than we thought.

[cristiancavalli]

@jmdobry Nice catch. Given that, I'm fine with @SkeLLLa 's current solution.

[jmdobry]

LGTM

[cristiancavalli]

Thanks @SkeLLLa !!

[SkeLLLa]

@cristiancavalli as for antipatterns - each module you require contains "risk". Even such little module as noop. Also noop module contains only one function that's already exist in javascript out-of-the-box. It makes no sense to write a separate module that will export something like

module.exports = Array.prototype.map;

The same with noop. But it's my point of view.

@cristiancavalli, @jmdobry Thanks tou you, too.