Skip to content

Instantly share code, notes, and snippets.

@ericelliott
Last active March 25, 2021 10:27
Show Gist options
  • Star 49 You must be signed in to star a gist
  • Fork 2 You must be signed in to fork a gist
  • Save ericelliott/e994ee541d0ed365f5fd to your computer and use it in GitHub Desktop.
Save ericelliott/e994ee541d0ed365f5fd to your computer and use it in GitHub Desktop.
Let's fix `class` in ES7

Two Simple Changes to Simplify class

I'm not suggesting drastic action. I don't want to break backwards compatibility. I simply want to make the class feature more usable to a broader cross section of the community. I believe there is some low-hanging fruit that can be harvested to that end.

Imagine AutoMaker contained class Car, but the author wants to take advantage of prototypes to enable factory polymorphism in order to dynamically swap out implementation.

Stampit does something similar to this in order to supply information needed to inherit from composable factory functions, known as stamps.

This isn't the only way to achieve this, but it is a convenient way which is compatible with .call(), .apply(), and .bind().

There are also other compelling reasons to refactor from a class to a factory, including the desire to utilize object pools instead of fresh instances, etc...

const AutoMaker = {
  Car (bundle) {
    return Object.create(this.bundle[bundle]);
  },

  bundle: {
    premium: {
      getOptions: function () {
        return ['leather', 'wood', 'pearl'];
      }
    }
  }
};

// The refactored factory expects:
const bar = AutoMaker.Car('premium');

// But since it's a library, lots of callers
// in the wild are doing this:
const oldCar = new AutoMaker.Car('premium');

// Which of course throws:
// TypeError: Cannot read property 'premium' of
// undefined at new AutoMaker.Car

The problem is that the new keyword hijacks this.

This is a really important problem to solve, because refactoring wrong class hierarchies into more composable pieces is a very common activity. I believe this problem is big enough that it's worth solving in the language spec.

For instance, we could deprecate new and drop the new requirement for ES7 class. Old classes would still be hard to refactor, but new ones would be significantly improved.

Since it wouldn't require a breaking change, I consider this to be low hanging fruit, ripe for the picking.

But we could go further:

Some people will be expecting things like instanceof to "work" with classes. The problem is that instanceof works well enough, until it doesn't. We train programmers to rely on it and then when they try to use it across execution contexts, iFrames, etc..., they get frustrated and stuck.

I recently ran across an otherwise awesome looking open source library that relied on instanceof and broke across iFrame boundaries in a pretty horrible way. Luckily, there were alternatives on npm that were not broken. Now I'm wishing I made a note of the library name and GitHub issue...

IFrames are not the only place it breaks. We expect something called instanceof to make some guarantees that might be pretty easy to statically analyze, but it relies on a prototype property that is dynamic. Oops.

Hence my suggestion that we also deprecate instanceof.

If we deprecate new and instanceof, it would go a long way to make constructors more usable (and by extension, classes).

@arackaf
Copy link

arackaf commented Mar 12, 2015

It does seem like an incredibly smart idea to NOT have classes invoked with new. The ability to transparently change from a class to a factory without any change to the callers seems like a no brainer now that you mention it.

Doing such a refactoring still seems like a rare event to me, but maybe we just code different types of application :)

@wmadden
Copy link

wmadden commented May 5, 2015

I realize it's not a language-level solution to the problem but you could adopt Ruby-style instantiation syntax instead:

let AutoMaker = {
  Car: { 
    new (bundle) {
      return Object.create(this.bundle[bundle]);
    },

    bundle: {
      premium: {
        getOptions: function () {
          return ['leather', 'wood', 'pearl'];
        }
      }
    }
  }
};

// The refactored factory expects:
let bar = AutoMaker.Car.new('premium');

// In the wild, nobody will be dumb enough to do this:
let oldCar = new AutoMaker.Car.new('premium');

@uniqname
Copy link

In the wild, nobody will be dumb enough to do this:

Until of course, they are. The problem is context switching and cognitive resource drain. Autopilot could easily account for such code.

Removing the additional context switching of new sounds like a win to me. However, due to legacy code dating to the beginning of JS-time, implementation support for new would likely need to continue in perpetuity, despite a language deprecation.

@ericelliott
Copy link
Author

The problem is that all ES6 classes require callers to invoke with new. They all throw errors if new is left off. There is a proposal for ES7 that would allow classes to specify what happens when the class is invoked without new.

That would help to make new optional, and then all callers will be able to leave it off.

The trouble is that specifying that behavior will add boilerplate to every new class definition. Sadly, I think most people simply won't bother.

I realize it's not a language-level solution to the problem but you could adopt Ruby-style instantiation syntax instead...

This doesn't even address the problem of refactoring from a class implementation to a factory implementation. All you're doing here is adding cluttering syntax to a factory that starts out a factory.

The problem we're trying to solve is that we started out with a class implementation, and now we need a factory implementation, but we can't make the switch, because lots of existing code already uses let oldCar = new AutoMaker.Car('premium');.

In other words, the only way forward is to create a new factory with a completely new name, deprecate the old constructor, update all your documentation, and then spread the word to the user community that there's a new, improved way to do it. In practice, that can be a monumental effort, especially if your API is popular and there are thousands or tens of thousands of users -- none of whom you have contact information for.

I've seen these situations in real codebases, and the class -> factory refactor is common enough that it made it into Martin Fowler's "Refactoring" book.

@dmin
Copy link

dmin commented Oct 8, 2015

Instead of refactoring every call site to handle your change to a factory function, wouldn't another option be to have your new factory function handle invocations that use new? I realize this is not ideal, and that it adds complexity and a dependency to the factory function, but I just wanted to play devils advocate to your statement that "the only way forward is to create a new factory with a completely new name..."

Something alongs the lines of:

'use strict';

var AutoMaker = {
  // ES2015 concise methods don't get a lexical name, so need to use long form
  Car: function Car(bundle, defaultContext = AutoMaker) {
    // Set context manually if invoked with 'new',
    // but allow using bind/apply/call
    const context = this.constructor ===  Car ? defaultContext : this;
    return Object.create(context.bundle[bundle]);
  },

  bundle: {
    premium: {
      getOptions: function () {
        return ['leather', 'wood', 'pearl'];
      }
    }
  }
};

// Factory function works as expected
let bar = AutoMaker.Car('premium');

// And can handle call sites in existing code which use 'new'
let oldCar = new AutoMaker.Car('premium');

Still, I certainly agree that it would be beneficial to drop the new requirement for ES7 class.

@spion
Copy link

spion commented Oct 9, 2015

How about this solution?

'use strict';

let AutoMaker = {
  get Car() {
    var self = this;
    return class Car {
      constructor(bundle) {
        return new self.bundle[bundle]
      }
    }
  },
  bundle: {
    premium: class Premium {
      getOptions() { return ['leather', 'wood', 'pearl']; }
    }
  }
}

let bar = new AutoMaker.Car('premium')
console.log(bar, bar.getOptions());

let bar code is unchanged. Classtory (class based factory) polymorphism works thanks to getter. Classtory itself works thanks to the ability of constructors to return entirely different objects.

Are you absolutely certain that there is any need whatsoever to refactor to factories in ES6? There is no (or very little) difference in capabilities between classes and functions...

@ericelliott
Copy link
Author

@spion: That solution doesn't work. Car is not defined.

@ericelliott
Copy link
Author

@dmin

wouldn't another option be to have your new factory function handle invocations that use new?

This is a breaking change. instanceof doesn't work anymore. Any callers relying on it will break.

@spion
Copy link

spion commented Nov 9, 2015

@ericelliott as a matter of fact, I tried this using node (v4.2, and v5) as well as babel-node, and it does work in all of them:

% node classtory.js
Premium {} [ 'leather', 'wood', 'pearl' ]

Did you try running it?

@brettz9
Copy link

brettz9 commented Jan 20, 2016

As far as instanceof, how about introducing a new isA operator and recommend it over instanceof without needing to break anything now or in the future? Likewise I hope any possible deprecating of new would only be with the new Class constructors. Let's try to keep old apps and libraries runnable indefinitely; JavaScript is of course not only for bells and whistles, so breaking changes are liable to break content as well, and while it's still possible, I hope we can avoid breaking access to old files and sites.

@dmitriz
Copy link

dmitriz commented May 15, 2016

I am totally sold on this idea ;)
Why not simply:

// create closure to keep private things private
var NewAutoMaker  =  (function() {

  var bundle = {
    premium: {
       // ... Implementation details ...
    },
    cheap: { 
       ... 
    },
    ...
  }

  return { 
    MakeCar: function (option) {
      return Object.create(bundle[option])
    };
  }

})();

const bar = NewAutoMaker.MakeCar('premium');

It is really hard to see any benefits using this and new here, that create more problems than solve.

Now the legacy AutoMaker can be supported if needed for some while,
until it isn't (which seems like a prevalent trend in software anyway).

@frankandrobot
Copy link

// But since it's a library, lots of callers
// in the wild are doing this:

Wait. So one issue is that callers don't read the API docs? This sounds more like a training/evangalism issue than a language issue.

I recently ran across an otherwise awesome looking open source library that relied on instanceof and broke across iFrame boundaries in a pretty horrible way

Playing the devil's advocate, one could argue that iframes should be avoided anyway. (For example, you probably wanted a web component instead of an iframe.) Out of curiosity, what are the other contexts where instanceof is broken?

@nfantone
Copy link

nfantone commented Nov 3, 2016

Out of curiosity, what are the other contexts where instanceof is broken?

@frankandrobot I wouldn't call this exactly "broken" as it's totally on par with the specification, but instanceof is very easily fooled into producing false negatives and positives.

function fn() {}
var foo = { life: 42 };
fn.prototype = foo;
var bar = Object.create(foo);
bar instanceof fn // true.

I'd argue that it's pretty unreliable, to say the least.

@felipediogo
Copy link

felipediogo commented Jul 11, 2018

In my project they're doing like this:

class Foo {
  bar(something) {
    doSomething(something);
  }
}
module.exports = new Foo();

It doesn't look good to me, what do you think?

@moimikey
Copy link

In my project they're doing like this:

class Foo {
  bar(something) {
    doSomething(something);
  }
}
module.exports = new Foo();

It doesn't look good to me, what do you think?

if you're into singletons and if i recall correctly, singletons are an anti-pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment