serby/ctrl

Do we really need _ or lodash?

Opened this issue · 13 comments

bengourley commented

Project wide search for _. returns 17 matches.

  • 16 of them are _.extend()
  • 1 of them is _.pluck()

I think it would be better to use an extend module (it probably exists, if not we can create it):

var extend = require('extend')
extend({}, defaults, options)

We could do the same for pluck, though as it's only used in on place it might make sense to just do an array map (which is what _.pluck does anyway). From the _ source:

_.pluck = function(obj, key) {
  return _.map(obj, function(value){ return value[key]; });
};

_ makes sense on the frontend, as it fills a lot of missing functionality from old browsers, but I think it's an unnecessary grab bag for node code.

serby commented

Not sure there is any real justification in that post. I am totally bought into the substack pattern and will always code my modules in that way. But I'm not fussed about refactoring out underscore because I trust underscore/lodash from a author, maintance, performance and major bug perspective. I don't want to do the duediligance on every module replacement for every function I use in underscore. Even if I do the duediligance, it would take some time to have the same confidence of these modules in production. When every utility function is a module it introduces more and more authors that we become reliant on, if one of them become lazy or lets a critical module fall into disrepair we are then forced to pick up it up and maintain it going forward. I accept this risk when there is no alternative, but do I really want to exacerbate this by forcing out utilitybelts like underscore out my projects for purist ideals when it does cause any problems. Modulatity is so so important, but there becomes a point when it is like what in OOD we call Object Oriented Masterbation http://robert.accettura.com/blog/2008/12/16/object-oriented-masturbation/ i give an example here https://speakerdeck.com/serby/building-for-clients-with-nodejs?slide=29 maybe it is a little contrived but you get the point.

In ctrl we've striped out all features so very little of underscore it used, as bundles are added more functions will be required.

It is a complete regression from my purist ways as a SOLID developer, underscore is exemply locical cohesion and at odds with the S, but I think there are practical considerations beyond SOLID when there is a pragmatic motive is to 'get shit done' and sometime being a purist is as more costly than making a few consessions.

I suspect that in time we will be able reach the highest of standards, but for now I'm not sure the benefit is justified.

evlun commented

+1

If we're willing to drop support for v0.6, we really should adopt util._extend() instead.

https://github.com/joyent/node/blob/master/lib/util.js#L563-573

serby commented

Many of the dependent modules that i control have already had that change
if they only use _.extend. But it isn't in the node spec so i'm not sure it
is a very wise move to use util._extend, I'm planing to replace in the
future with a more robust fix.

Sent from a telephone

On 14 Nov 2012, at 22:29, Erik Lundin notifications@github.com wrote:

+1

If we're willing to drop support for v0.6, we really should adopt
util._extend() instead.

https://github.com/joyent/node/blob/master/lib/util.js#L563-573

—
Reply to this email directly or view it on
GitHubhttps://github.com//issues/134#issuecomment-10388781.

bengourley commented

Not sure there is any real justification in that post

The justification is that we are depending a module that is 4279 lines of code (1MB npm package), of which we are using 11 lines (0.002%).

but there becomes a point when it is like what in OOD we call Object Oriented Masterbation

I completely agree with that notion, but I don't think it applies to this instance. No abstraction is happening. In fact less abstraction is happening. You require() a function that you intend to use, rather than a grab-bag (object) containing functions that you probably won't use.

Even if I do the duediligance, it would take some time to have the same confidence of these modules in production.

This doesn't have to introduce any new code, we can just use the same function that's in underscore anyway. Plus, a smaller module is more easy to test and isolate issues. We can reuse the underscore/lodash source and tests.

In ctrl we've striped out all features so very little of underscore it used, as bundles are added more functions will be required.

We can't leave things in based on the premise that future bundles might need them, otherwise we'd include the whole of npm! Dependencies can be added as and when they are needed (and to each bundle's own package.json).

If we're willing to drop support for v0.6, we really should adopt util._extend() instead.

To echo what Paul is saying, I thought about using util.extend, but it is private API stuff (denoted with the '') and undocumented. It's not future proof.

serby commented

So the justification is for purism sake not any actual real world problem
that we are facing. Which was my point.

I in fact created a package to do just extend right from the start, see
piton-mixin. All it did was mean I had more work to do on each node
release, not because the code itself went out of date but because, expresso
became deprecated in favour of mocha, times that by the number of functions
we use in underscore (about 10) and it becomes a real problem not just a
hypothetical one.

Using underscore meant I spent less time wanking about and more time being
pure in the areas that real mattered.

To be clear, I would like a no headache pure fix to this. Maybe ever adding
common underscore functions (pluck, max, min, extend, shuffle, union,
intersect, difference, uniq) to the fat 'utility belts' that are the Array
and Object prototypes of JavaScript. If you want to write and maintain a
separate npm module for each of the functions then feel free. It is a lot
of effort and doesn't really progress the project. What's worst is as soon
as a developer using ctrl to build their project installs underscore,
because they hadn't spotted the pluck module already included along with a
100 other modules, you have duplication and the same function being done in
two different ways, a code smell worst than logical cohesion IMO.

Sent from a telephone

On 14 Nov 2012, at 23:18, Ben Gourley notifications@github.com wrote:

Not sure there is any real justification in that post

The justification is that we are depending a module that is 4279 lines of
code (1MB npm package), of which we are using 11 lines (0.002%).

but there becomes a point when it is like what in OOD we call Object
Oriented Masterbation

I completely agree with that notion, but I don't think it applies to this
instance. No abstraction is happening. In fact less abstraction is
happening. You require() a function that you intend to use, rather than a
grab-bag (object) containing functions that you probably won't use.

Even if I do the duediligance, it would take some time to have the same
confidence of these modules in production.

This doesn't have to introduce any new code, we can just use the same
function that's in underscore anyway. Plus, a smaller module is more easy
to test and isolate issues. We can reuse the underscore/lodash source
_and_tests.

In ctrl we've striped out all features so very little of underscore it
used, as bundles are added more functions will be required.

We can't leave things in based on the premise that future bundles might
need them, otherwise we'd include the whole of npm! Dependencies can be
added as and when they are needed (and to each bundle's own package.json).

If we're willing to drop support for v0.6, we really should adopt
util._extend() instead.

To echo what Paul is saying, I thought about using util.extend, but it is
private API stuff (denoted with the '') and undocumented. It's not future
proof.

—
Reply to this email directly or view it on
GitHubhttps://github.com//issues/134#issuecomment-10390442.

lukewilde commented

Perhaps naively, I think controls biggest problem is currently that it isn't finished. If lodash is offering a means to speed up development my vote is to leave it in, for now. In the long run, I can see how it may be underutilised in the core and personally like the idea of removing it. I would suggest finishing the core and then look at refactoring it out after release.

tomgco commented

At this moment in time I am agreeing with @lukewilde on this one on leaving it in at the moment.

We do not know what will be needed in newer iterations of the codebase, and by leaving lodash in does enable rapid development; for core and for bundle development.

Underscore is the most popular module, and I would like to think one which has a lot of eyes on the codebase. This means us and the community can trust the functionality of this library.

Maintaing our own versions of these functions just so we do not have to depend on a monolithic collection of functions does sound nice, but in practise most likely will be much more of a ball-ach than adding a measly 1MB module.

In all, I think we should stick with lodash at the moment, and maybe in the future we can re-evaluate our uses for it.

bengourley commented

Dammit. My trackpad freaked out and I hit close and comment before I'd finished typing…

If the consensus is that it should stay, then obviously it should stay. I just can't see the reason for keeping dependencies 'just in case'. It takes seconds to do npm install --save lodash if a developer decides that they need it (the --save option writes the dependency to package.json so you don't even have to do that).

I'm not trying to hate on underscore/lodash here, I think they do a great job. And I am pretty certain that we will keep it around for front-end stuff, as it fills a lot of useful functionality that's missing in browsers. However, the things that make it good for the browser render a bit pointless in Node – you know exactly the environment you're in and what native functions are available, yet a big portion of the library is defining forEach(), map(), keys() etc., functions which already exist.

In the end, I'm not particularly fussed if it stays. There are bigger fish to fry, and it doesn't have a tangible negative impact on the application (except some extra waiting around at install/deployment time). I think, though, that it would have been an oversight for us not to ratify its existence when such a tiny portion of it is being used.

serby commented

'except some extra waiting around at install/deployment time' If each of the 9 functions I listed was a module, the combined weight of the code, the package.json, the tests, readme and the fact that each would be over a new HTTP connection is likely to be equal if not greater than the size/speed of underscore.

bengourley commented

9 functions? We use 2? Anyway, 9 functions would not be a meg and npm does http reqs in parallel.

serby commented

That's only because everything is striped out of ctrl. There are ~9 in
regular usage at Clock.

Also _.pluck uses _.map. _.min and _.max use _.each. _.shuffle uses
_.random. _.union uses _.uniq which uses _.contains. _.intersection uses
_.filter. etc. The dependency tree grows and grow until you pretty much
have the entire of underscore. Underscore is good enough from
a cohesion point of view to make it hard to pick apart.

If you can find drop-in replacements for at least:
extend, pluck, uniq, shuffle, that are maintained by people as established
as documentcloud then maybe it would be worth looking at further. But I'm
becoming more and more convinced that it is a non-issue and a purest debate
not worth having.

Should we also replace async because we only use 'parallel' in ctrl?

On 16 November 2012 17:24, Ben Gourley notifications@github.com wrote:

9 functions? We use 2? Anyway, 9 functions would not be a meg and npm does
http reqs in parallel.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/134#issuecomment-10454575.

paul.serby@clock.co.uk
Chief Technology Officer

Direct: +44 2030 516115__Mobile: +44 7881 550999
*

*

bengourley commented

Underscore is good enough from a cohesion point of view to make it hard to pick apart.

I can see these ways in which it could be decoupled:

  • Array functions
  • Object functions
  • Function functions
  • Template language

Not in the browser, because they all tend to rely on the ES5 polyfills, but in Node I think that would definitely make sense.

Should we also replace async because we only use 'parallel' in ctrl?

If the goal was to be 'pure', yes. But using 1/20 functions in a much smaller library is more acceptable (for me).