Implement stampit/convertConstructor utility function

[koresar]

There is one last bit left to implement in order to fulfill the Stampit v3 RFC - the convertConstructor function.

I'm seeking for a volunteer to implement that. It should convert ES6 classes to stamps. The initializer, properties, methods, and staticProperties should be segregated from a ES6 class and added to a stamp.

Here is how the stampit v2 had convertConstructor implemented: https://github.com/stampit-org/stampit/blob/v2.1.2/src/stampit.js#L275

[koresar]

So... We have a problem.

I examined the ES6.

Here is what we should implement in order to convert ES6 classes in a fully compatible way.
This code should produce the picture below:

    class Point {
        constructor(x, y) {
            this.x = x;
            this.y = y;
        }
    }

    class ColorPoint extends Point {
        constructor(x, y, color) {
            super(x, y);
            this.color = color;
        }
    }

    let cp = new ColorPoint(25, 8, 'green');

Stamps do not allow prototype chaining. By design. Deliberately.
This means there is no way we can write a fully fledged convertConstructor.

Ok. but we can try supporting single level (no inheritance) classes. Right?

No. We can't do that either. ES6 classes throw exception if you try calling its constructor without the new keyword: Cannot call a class as a function

Ok, but we can try supporting the "class-like functions". Can we? Yes we can. The same way as before.
But we must make sure people are not using convertConstructor for ES6 classes. I would better rename the convertContructor to convertClassLikeFunction to make sure there is no confusion.

See that branch and that file if you wanna play around with it:
https://github.com/stampit-org/stampit/blob/convert-constructor/test/convert-constructor.js
Just run npm test

[danielkcz]

I am probably missing something in my brains, but I don't see a problem here.

const convertConstructor = ctor => stampit({
    init: function (_, {args}) {
        return Reflect.construct(ctor, args);
    },
    methods: ctor.prototype,
    statics: ctor
});

const ColorPointStamp = convertConstructor(ColorPoint);
const cp = ColorPointStamp(25, 8, 'green');

What's the catch here? The init gets called, constructor gets all necessary arguments and prototype chain will work from there. We don't need to do anything about that really.

Maybe you are missing the piece with Reflect.construct? It's exactly made to create instances of ES6 classes dynamically.

[danielkcz]

Here is a codepen with this simple use case working like magic ✨ 😉

See the Pen Playground for stampit by Daniel K. (@FredyC) on CodePen.

<script async src="//assets.codepen.io/assets/embed/ei.js"></script>

[JosephClay]

Awesome use of Reflect. I always wondered how to get around the new problem with arguments. Here's the pertinent line of code from a reflect shim.

return new (Function.prototype.bind.apply(newTarget, [null].concat(args)));

@FredyC Tried it, can't get it to break. Are there edge-cases where this won't work?

[danielkcz]

I haven't actually tried to run it against those test cases from above: https://github.com/stampit-org/stampit/blob/convert-constructor/test/convert-constructor.js

Can someone else try that please?

[koresar]

After a long chat we decided to go this way:

1. The convertConstructor will be used for the ES5 class-like functions.
2. The new convertClass will be used for ES6 classes:
   2.1) In the API docs we'll warn that only the non-inherited classes are fully supported
   2.2) and the classes inherited at least once are loosely supported (list the limitations).

Here is a rough WIP implementation (run it here):

const convertClass = ctor => stampit.default({
    init: function (_, {args, instance}) {
        return Reflect.construct(() => instance, args, ctor);
    },
    methods: ctor.prototype,
    statics: Reflect.ownKeys(ctor)
        .reduce((statics, k) => {
            if (k !== 'length' && k !== 'name' && k !== 'prototype')
                statics[k] = ctor[k];
            return statics;
        }, {})
});

[Sigfried]

Can you tell me where this stands? I just ran across stampit a couple hours ago. It looks like it solves some problems I'm having very nicely. Unfortunately I have a tight deadline and need to mush together a number of classes. Do I have to rewrite them all as stamps?

[koresar]

Hi @Sigfried

Thanks for the question. The work has stuck at some point of time: https://github.com/stampit-org/stampit/compare/convert-constructor?expand=1
This requires more unit tests. And probably implementation improvements...

I'm planning to implement it as the @stamp/convert-constructor module later. If you really need stamps - your only option atm is to convert classes to stamps manually. Sorry about that.

[danielkcz]

@Sigfried I wouldn't be so negative about it as @koresar. Yes, there is no official support for it, but if you go through comments here, you can just as easily make an implementation that suits your needs. With stamps there is no need to wait for maintainers to finish something as long as specification stays same. You can just create your own stamp and use it in your project.

[Sigfried]

Thanks for the quick replies!

I'm seeing there's not much work to manually converting my classes to stamps (though I'd rather not since some old code will still use the class versions), so I'm able to move ahead, and stampit is saving me from making a much bigger mess than I would have otherwise.

Another dumb question: there's no way to write stampit methods as arrow functions, right? My ES6 classes use arrow functions for methods (which nicely binds them to their instances), so the main work in converting them to stamps is changing arrow functions to the regular kind. ...So now I'm wondering, are stamp methods bound to instances? (I.e., detached=instance.foo; detached() only works with this===instance for arrow methods.)

[koresar]

@Sigfried could you please show us some code regarding

My ES6 classes use arrow functions for methods

I am not sure there is such syntax in ES6.

[danielkcz]

@koresar You can actually use fat arrow in classes with class properties, just like...

class MyClass {
  myBoundMethod = () => {
  }
}

It's ugly as hell, but it works.

@Sigfried Unfortunately, you cannot really do this with stamps because of there no such support from the language. There is a more extensive discussion in this issue.

[ericelliott]

@FredyC Class properties is still in the early proposal stage for a future version of JavaScript. It is not part of the current standard.

[danielkcz]

And why does that matter? It's more of the question if person/company is comfortable using Babel. As long as they keep a version that supports that feature, it will just work no matter the standard. I don't want to flame about it, but it's kinda ridiculous to stop at things that are standardized only.

[koresar]

@FredyC I can clearly see that @Sigfried is using ES6 classes, not ES-Next or something:

My ES6 classes use arrow functions

[danielkcz]

Eh, what? :) He said that he is using arrow functions in classes, that kinda means that he is using these non-standardized class properties or am I missing something here? :)

Edit:
Also, the term "ES6 classes" is just so inaccurate and most of the time it just refers to classes generally, not to some feature set that is used. Nobody is talking about ES8 classes or ES2017 classes. Most of the people are not really watching when was what feature standardized.

Edit2:
You are discussing here something completely irrelevant, I just wanted to point that such syntax exists because you were confused about it.

[Sigfried]

Sorry for the delay. Yeah, I was doing stuff like:

cids = () => this.prop('cids')
concepts = (cids=this.cids()) => concepts(this.conceptState.loaded).filter(c=>_.includes(cids,c.concept_id))
concept_ids = (cids) => this.concepts(cids).map(d=>d.concept_id)
cidCnt = () => this.cids().length
conCnt = () => this.concepts().length

I like the arrow methods, but it's not a big deal. I've already converted everything.

[koresar]

So, Daniel was right - you are actually using Babel! I must mention, that this keyword in the following code is VERY misleading.

cids = () => this.prop('cids')

According to ECMAScript standard in this case this should not point to the object instance, but to the upper scope - likely the window or global scope in your case.

I assume that Babel compiles it wrong. :) Not sure.

P.S.
This is more readable to me, my colleagues, and non JS developers. Also, it is part of ES2015:

cids() { return this.prop('cids') }

Compare to yours:

cids = () => this.prop('cids')

P.P.S.
I am starting to hate transpiling more❗️

[Sigfried]

Sorry, I wasn't clear. All that stuff I quoted was in the context of a class definition:

export class ConceptSet {
  constructor() {
    ...
  }
  cids = () => this.prop('cids')
  concepts = (cids=this.cids()) => concepts(this.conceptState.loaded).filter(c=>_.includes(cids,c.concept_id))
  concept_ids = (cids) => this.concepts(cids).map(d=>d.concept_id)
  cidCnt = () => this.cids().length
  conCnt = () => this.concepts().length
  ...
}

I'm not going to defend transpilers, but I like this experimental feature: https://babeljs.io/repl/#?babili=false&evaluate=true&lineWrap=false&presets=es2015%2Creact%2Cstage-2&targets=&browsers=&builtIns=false&debug=false&code_lz=MYGwhgzhAEAKD2EAuBJAdgM3tApgDyRzQBMYAlHMYJAOgGF4BbAB3jSKWgG8AoASAAWYEiBwB5ZkgCWbCACEArkiRs6IKcADW0ALzQAFDgCUugHzce0K9CQCpEGhBxIAykjCF9XCAPgB3CWlZAFl4YjAQAC4bACcFHABfIwBuS2gEngSgA&experimental=true&loose=false&spec=false

The this is not misleading. It does refer to the instance. And, the great thing (besides saving me a few keystrokes) is, whereas in your version,

cids() { return this.prop('cids') }
let foo = instance.cids
foo()

will fail unless you attach foo to an object, like:

let foo = instance.cids.bind(instance)

But

cids = () => this.prop('cids')
let foo = instance.cids
foo()

works as one would hope, with this referring to instance.

[ericelliott]

cids = () => this.prop('cids')
let foo = instance.cids
foo()

works as one would hope, with this referring to instance.

I have not read the class property spec, but if it works like object props, the transpiler is probably making a mistake. this can't be bound in real ES6 arrow functions because they don't have a slot for this -- in other words, they always delegate this to lexical scope.

[ericelliott]

Oops. didn't mean to close.

[Sigfried]

But it's defined inside the class definition, so can that make the lexical scope be the instance?

[danielkcz]

It's really strange. I wasn't able to actually find the specs of this experimental feature. Babel has it implemented and people are using it. There is short mention in a Babel blog which reasons about it like...

The body of ES6 arrow functions share the same lexical this as the code that surrounds them, which gets us the desired result because of the way that ES7 property initializers are scoped.

It almost looks like that it's just a feature that Babel came up with and it's not standardized actually. 😖

There is also this comment on StackOverflow that kinda makes sense to me...

I think this is because it would create a new weird part in the language: you would have class methods whose execution context is the context the class is declared in. Class methods are, by definition, to be called on the instance as a context. If you want a method that does not care about context, create a static one, which can be declared as an arrow function directly in the class definition

Btw to make things even more confusing, there is a proposal for private fields ... 📢

[koresar]

Thanks for the info @Sigfried

Just a quick note. The approach is memory heavy. This attaches methods not to the prototype, but to the instance itself.

export class ConceptSet {
  cids = () => this.prop('cids')
}

If you are creating a lot of these objects - you are wasting CPU and memory and GC etc.

[danielkcz]

@koresar And isn't that what we are doing in stamps whenever we need to bind method, make it private or access stamp configuration? In those cases, we also have to attach to the instance. I don't see why you should warn about it for classes.

[koresar]

@FredyC you are right. But Sigfried do not need method binding and private props.

[Sigfried]

I don't have time to mess with it right now, but I was thinking I could do the same with stamps by using props instead of methods.... Can't recall if I had an idea of where 'this' would come from..... sending from phone. please pardon terseness, typos, incoherence, etc. On Jun 18, 2017 5:52 AM, "Vasyl Boroviak" <notifications@github.com> wrote: @FredyC <https://github.com/fredyc> you are right. But Sigfried do not need method binding and private props. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#257 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABg2850Y8h4g2HB2OHGOCkzu5F5Atrz0ks5sFPNWgaJpZM4KTjy9> .

[koresar]

Sigfried, you can surely do the same using stamps.

import {init} from `stampit`;
export const ConceptSet = init(function () {
  // ... constructor code here ... //

  this.cids = () => this.prop('cids')
  this.concepts = (cids=this.cids()) => concepts(this.conceptState.loaded).filter(c=>_.includes(cids,c.concept_id))
  this.concept_ids = (cids) => this.concepts(cids).map(d=>d.concept_id)
  this.cidCnt = () => this.cids().length
  this.conCnt = () => this.concepts().length
});

It is exactly the same implementation as the Babel transpiled code you referenced above.

[Sigfried]

Totally extra cool!!!

I was just being dumb wondering out how to shoehorn those into methods or properties.

I may not need to use methods or properties at all, right? The important work (at least for me) that stampit is doing is composing without inheritance insanity.

So, if I put everything in init like your example, it'll all get merged correctly, right? (Assuming I don't put crazy nested stuff in there...)

Thanks!

[koresar]

Correct.

But I still would recommend using methods() if you are creating a lot of these objects.

Always glad to help!

[gajewsk2]

So I have to figure out how to do a conversion of an old model to stampit. This seems... bad, but does do the behavior I would want w/ ES6 classes stop-gap-until-converted + stampit:

import stampit from 'stampit';
import PubSubModel from './pub-sub.model';

// The conversion
const oldskool = stampit().init(function() {
  Object.setPrototypeOf(this.__proto__, new PubSubModel());
});

// Now you can compose with it just like any other stampit factory...
window.myThing = stampit.compose(oldskool).methods({
  bar: function bar() { return this; },
  // your methods here...
});

export default oldskool;

If you just use Object.setPrototypeOf(this, new SomeES6Constructor()), then everything that stampit composed (bar in the case above) gets blown away. Is what I'm doing really that dangerous since __proto__ and setPrototypeOf are both in the ES2015 spec?

Is there a better way to do this currently?

[koresar]

Sorry, what do you mean exactly when saying "blown away"?
Here is your code (I've shortened it a bit) running in node.js:

const {init} = require('stampit');
function PubSubModel () {
  this.a = 1;
}
PubSubModel.prototype.foo = function () {
  console.log('foo')
}

// The conversion
const oldskool = init(function() {
  Object.setPrototypeOf(this.__proto__, new PubSubModel());
});

// Now you can compose with it just like any other stampit factory...
const myThing = oldskool.methods({
  bar() { return this; },
  // your methods here...
});

myThing().bar().foo()

This code prints foo to the console as expected.

[gajewsk2]

Sorry, what do you mean exactly when saying "blown away"?

Try taking off the .__proto__ on this.__proto. The stamped methods no longer are present. So yes, our code works as expected. myThing().bar().foo()

My question is, is this a good way to deal with es6 class to stamps until this feature is fully implemented?

[koresar]

A tip.
If you're looking to add EventEmitter (aka PubSub) to your objects then you might want to take a look at the @stamp/eventemittable package.

import stampit from 'stampit'
import EE from '@stamp/eventemittable'

const MyStampThing = stampit(EE, {
  init...
  props...
  methods...
  etc...
});

The implementation of the @stamp/eventemittable is very simple. Here is the full source code:

import {EventEmitter} from 'events';

export default stampit({
  statics: {
    defaultMaxListeners: EventEmitter.defaultMaxListeners,
    listenerCount: EventEmitter.listenerCount
  },
  methods: EventEmitter.prototype
});

[koresar]

I see what's the issue in your case. Here is a 6 minutes article I would highly recommend you to read: https://medium.com/@koresar/fun-with-stamps-episode-1-stamp-basics-e0627d81efe0
Especially see the Breaking down a stamp into pieces part.

It explains that every object instance you create from a stamp (stamps are the myThing or oldskool in your case) have a __proto__, and stampit puts all your methods to the proto.

const Stamp = stampit().methods({ myMethod() {} });
const objectInstance = Stamp();
console.log( Object.getPrototypeOf(objectInstance).myMethod );

prints [Function: myMethod]

So, this line overwrites all the methods of your object instances:

Object.setPrototypeOf(this, new PubSubModel());

[gajewsk2]

Thanks for the responses. I definitely plan on going through that whole tutorial set.

However, I'm actually not saying that the code I posted is an issue or that I don't understand why it's working how it's supposed to. It's doing exactly what I expect and want (i think, I don't mess with proto directly often).

My question/issue is: According to the api notes eventually we'll have a standard stampit way to handle this. Removed convertConstructor(). We plan to revive it and support the ES6 classes.
So what do we do for now? Writing to this. __proto__, even if it works, is it OK to do this? What is the best practice to convert es6 classes to stampit?

[koresar]

There is no "best practice". Every case is different. Here is a new version of a (non fully tested) implementation:

function classStaticProperies(ctor) {
  return Reflect.ownKeys(ctor)
  .reduce((statics, k) => {
    if (k === 'prototype') {
      Object.assign(statics, classStaticProperies(ctor.prototype))
    } else {
      if (k !== 'length' && k !== 'name' && k !== 'constructor') statics[k] = ctor[k];
    }
    return statics;
  }, {});
}

function convertClass(ctor) {
  return stampit({
    initializers: [function (_, {instance, args},) {
      Object.assign(this, Reflect.construct(ctor, args));
    }],

    methods: ctor.prototype,

    staticProperties: classStaticProperies(ctor),

    staticPropertyDescriptors: {
      name: {
        value: ctor.name
      }
    }
  });
}

Please, report bugs. :)

Usage:

import stampit from 'stampit';
import PubSubModel from './pub-sub.model';

// The conversion
const oldskool = convertClass(PubSubModel);

// Now you can compose with it just like any other stampit factory...
window.myThing = oldskool.methods({
  bar: function bar() { return this; },
  // your methods here...
});

export default oldskool;

[gajewsk2]

Ok, I played around with this a bit more.　And I'll be able to sleep better at night knowing I'm not directly accessing the __proto__ property, and instead leveraging my new best friend Reflect.

class PubSubModel {
  constructor(){this.a = 1;}
  foo(){console.log('foo')}
  get b(){return this.a;}
  set c(x){this.a = x}
  static d(){return 'baz'}
}


// The conversion
oldskool = stampit.init(function() {
  // Ought to do the trick
  Reflect.setPrototypeOf(Reflect.getPrototypeOf(this), Reflect.construct(PubSubModel, []));
});

// Now you can compose with it just like any other stampit factory...
myThing = oldskool.methods({
  bar() { return this; },
  // your methods here...
});

Wouldn't Reflect.setPrototypeOf(Reflect.getPrototypeOf(this), Reflect.construct(ctor, [])); instead of
Object.assign(this, Reflect.construct(ctor, args)); be better? Object.assign won't iterate proto methods and will lose access to foo in my example, if I were to use convertClass.

[koresar]

Sorry, I didn't test methods properly.

function classStaticProperties(ctor) {
  if (ctor === Function.prototype) return {};
  return Reflect.ownKeys(ctor)
  .reduce((statics, k) => {
    if (k !== 'length' && k !== 'name' && k !== 'prototype') {
      statics[k] = ctor[k];
    }
    return statics;
  }, classStaticProperties(ctor.__proto__));
}

function classMethods(ctor) {
  if (ctor.__proto__ === Function.prototype) return {};
  return Reflect.ownKeys(ctor.prototype).reduce((methods, k) => {
    if (k !== 'constructor') {
      methods[k] = ctor.prototype[k];
    }
    return methods;
  }, classMethods(ctor.__proto__))
}

function convertClass(ctor) {
  return compose({
    initializers: [function (_, {instance, args},) {
      Object.assign(this, Reflect.construct(ctor, args));
    }],

    methods: classMethods(ctor),

    staticProperties: classStaticProperties(ctor),

    staticPropertyDescriptors: {
      name: {
        value: ctor.name
      }
    }
  });
}

=====
Regarding your suggestion.

Usually an object instance created form stamp have this chain: objectInstance -> methodsProto.

You are suggesting to break into the prototype chain:
objectInstance -> instanceOf_PubSubModel -> methodsProto.

You've suggested to make the prototype chain longer. This is exactly what stamps are trying to solve - long prototype chains. Stamp philosophy is to avoid that.
Why? Just imagine you want to add one more class into the mix:
objectInstance -> instanceOf_PubSubModel -> instanceOf_UserModel -> methodsProto.
And one more:
objectInstance -> instanceOf_PubSubModel -> instanceOf_UserModel -> instanceOf_ConfigModel -> methodsProto.
etc.

That is very bad long term, although looks like an acceptable solution at the moment.

My code actually converts a class to a stamp. It preserves the objectInstance -> methodsProto philosophy.

I hope that answers.

[koresar]

@gajewsk2 I have implemented and published the official convert-class module: https://www.npmjs.com/package/@stamp/convert-class
npm i @stamp/convert-class

It fixes one bug in the code example above. So, you might want to use that npm package. :)

[koresar]

Since none is creating ES5-style "classes" any more, I propose to not implement the convertConstructor at all. The ES6 convertClass is enough nowadays I reckon.

WDYT?

If no objections I'd close this issue in few days.

[gajewsk2]

Thanks @koresar that seems like a very useful addition to stampit adoption. I'll be sure to try it out soon.

As to your other point, do we need convertConstructor if we have convertClass:
I've gathered that stampit is trying to be friendly to the ES5 ecosystem, since it's written in ES5. But if you don't support convertConstructor, then who is stampit targeting?
People with build-systems that already convert their es6+ classes to ES5? But then people's build system would be able to transpile stampit if it was written in ES6+ syntax.
People who use native ES6+ without any transpiling? But then why code stampit in ES5?

Personally, I would still benefit from a convertConstructor as I'm using stampit as a way to bridge both the old and new code in my code base, which has both ES6 and old "class", but I can write something to hold myself over indefinitely. I'm curious what breaks if I pass a plain old constructor w/ prototype chain (old school classes) to the new convertClass. Time to dig in and find out :)

[danielkcz]

I would assume that since not many people are really asking for a way of converting ES5 classes, it's no longer a pressing issue. In fact, stampit was written in ES6, but then @koresar got annoyed by a Babel and rewritten it :D There isn't much of reason today why to stay with ES5 except to support IE 11. But looking at these global stats I wouldn't be worried that much really :)

In my opinion, if you have an older project written in ES5, it's unlikely it's build using stamps and even more unlikely, stampit would be added there ad-hoc just for the kicks of it. The newer project will be likely written with ES6 or it might be just a small project which doesn't really care about composition. In overall there doesn't seem to be much of userbase of ES5 stampit imo.

[koresar]

@FredyC have just told the reasoning why stampit is ES5. Also, few other reasons are listed here: https://github.com/stampit-org/stampit/releases/tag/v4.0.0

If someone would rewrite stampit in ES6 it'd be awesome. There are few requirements though:

• Same <1.5kB gzipped size.
• The min.js bundle is ES5 compatible (including Object.assign polyfil).
• All tests still pass (this is the hardest part).
• And have the similar build dist(s) as Preact has:

  "main": "dist/preact.js",
  "jsnext:main": "dist/preact.esm.js",
  "module": "dist/preact.esm.js",
  "dev:main": "dist/preact.dev.js",
  "minified:main": "dist/preact.min.js",
  "types": "dist/preact.d.ts",

Frankly, that's a lot of work! And the benefits are rather small. Tiny modules like stampit do not need all that project infrastructure complexity. Ask Sindre Sorhus :) He was able to develop that myriad or modules because he does not transpile any of them. No transpilation saves time!

PS: I'd focus efforts on better documentation :) Because the hardest part about stampit is the mind shift from classes.
Look what I have just started!
https://stampit-org.gitbooks.io/docs/content/ (auto synced to https://github.com/stampit-org/docs)

I invite anyone to contribute to that gitbook. It's empty atm. Ask me for write access. :)

[danielkcz]

To be honest (and its way OT), I am not using stampit for the last year or so. It cannot be used with React functional components and for a business logic I am using mobx-state-tree which is somewhat similar to stamps, but reactive on top of that.

That being said, I don't really feel motivated to help with anything on this project right now. Sorry.

[koresar]

@gajewsk2 probably I need to note that convertContructor is not cancelled. :) Going to make it soon.