ericelliott/credential

Needs to be a factory for instance safety.

ericelliott opened this issue · 8 comments

ericelliott commented

New hotness:

var credential = require('credential');
var pw = credential(options); // instance safety FTW
ericelliott commented

No.

  1. Idiomatic Node. All core express / middleware are factories, not constructors.
  2. In ES6, calling a class without new throws a hard error. In ES5, calling a constructor without new causes lint warnings, and
  3. Using new couples all callers to the constructor method of object instantiation, so you commit two sins every time you export a factory: force tight coupling where there could have been no coupling, and force more typing by every caller with no beneficial gain.

If been challenging people for years to write a good blog post explaining what constructors and classes provide that factories don't do better. So far that challenge has gone unanswered.

tjconcept commented

You're saying this trick wont work anymore?

if (!(this instanceof Credential))
    return new Credential(opts);

If instead a function was exported:

module.exports = function( opts ){ return new Credential(opts); }

Would that fit?

ericelliott commented

Why use new at all? Seems like all you're doing is opting into unnecessary complexity.

tjconcept commented

I like prototypes :)

  1. I like the idea that when something is meant to be the same, it is actually the same. For instance if I have two objects that are supposed to have a shared property, I'd rather have them hold a reference to a shared object with the value than copying the value to both objects.
  2. I think it communicates the intentions of my code very well (readability)
  3. I think there could be a performance benefit
  4. Almost all of the top developers I try to learn from is using prototypes (e.g. bluebird, browserify and node itself)
  5. The only downside I can think of is the one line to have new be optional
ericelliott commented
  1. The whole purpose of making a factory is so that there can be instance safety, which means that each instance needs its own values, which are likely to be different from each other. Shared state is a recipe for bugs.
  2. What's more readable, an object literal, or a whole bunch of disconnected assignments to foo.prototype? I'll take the object literal, thanks.
  3. Factories that export object literals are much faster to instantiate than constructors that assign property values one at a time. How much faster depends on how many properties there are to assign. There is a very subtle benefit from hidden classes created by constructors for property access, but a tiny portion of the time spent running your program is executing JavaScript, and a miniscule portion of that tiny portion is spent accessing properties. On the slowest machines currently on the market, you can access properties millions of times per second without hidden classes. If you spend any time at all thinking about this, you're optimizing the wrong things.
  4. I've actually seen the numbers, based on GitHub projects. I wish I could remember the URL so I could link, but here's what's really the most common JS object instantiation strategies: Object literals (by a HUGE margin), Factories (regular function that returns an object, which most of the Node APIs do, and most of the popular libs like Express, jQuery, Underscore, etc...), THEN constructors, THEN ES6 class in dead last place. I expect ES6 class to jump to 2nd or 3rd place when more people adopt it.
  5. First, this is really a much bigger problem than people think. Class -> Factory is a common enough refactor that it made Martin Fowler's "Refactoring" catalog. The problem is, with radically different behavior depending on whether new and constructors are used, those refactors can be very hard. The problem with refactoring callers is that in many cases (libraries, public API's, etc...) it's impossible to know about all the callers, so you're left with no choice but to deprecate the class and create a totally new factory, and then you have the problem of educating users about the deprecation and new recommendation. This is really hard work and really expensive in both time and money.

One more thing, If I remember correctly, you can't have new be optional with ES6 class. );