Clone methods do not respect proper OOP

[kungfooman]

What you have right now all over the place is:

class Node {
  constructor(asd) {
    this.asd = asd;
  }
  clone() {
    return new Node(this.asd);
  }
}
node1 = new Node(123);
node2 = node1.clone();

However, you can show very easily how wrong it is if you actually try to use OOP:

class SpecialNode extends Node {
  broom = 'broooom';
}
specialNode1 = new SpecialNode(456);
specialNode2 = specialNode1.clone();

specialNode1 instanceof SpecialNode is of course true, but specialNode2 regresses into a Node... the proper way to clone in JavaScript is: new this.constructor(this.asd)

[frading]

Thank you @kungfooman . Do you think you could make a PR for just a single one of those classes, so I see how it looks in practice?

[kungfooman]

Do you think you could make a PR for just a single one of those classes, so I see how it looks in practice?

I would invest more time since this is an interesting project, but I realized I can't do anything with this based on the license.

Last time I checked ThreeJS got it wrong aswell, but PlayCanvas does it right, so you can take that as an example:

https://github.com/playcanvas/engine/blob/f30ec5736535de15afbf8d03632fd9eab03a7caa/src/math/vec3.js#L109-L122

Or a non-trivial case:

https://github.com/playcanvas/engine/blob/f30ec5736535de15afbf8d03632fd9eab03a7caa/src/framework/entity.js#L582-L630

I don't know how it fits your project, PlayCanvas is MIT license but also paid (for extra online editor abilities like project exporting). MIT license is usually attracting more contributors.

[frading]

Thanks a lot for those links.

There are indeed cases where I'm using this method to clone objects, like the following: (although those classes are never inherited):

polygonjs/src/core/animation/TimelineBuilder.ts

Line 141 in 782cdbc

clone() {

polygonjs/src/core/animation/PropertyTarget.ts

Line 18 in 782cdbc

clone() {

When it's with classes that do get inherited, using this.constructor gives this typescript error This expression is not constructable. Type 'Function' has no construct signatures.ts(2351). I'll investigate if it's safe to cast the type of the constructor in those situation.

And regarding the license, Polygonjs used to be MIT, but I've recently switched to Polyform Shield, mostly to forbid the creation of a competing node-based editor and marketplace. But it is restriction free otherwise. I'm still pondering if that's the right choice, as I definitely want to encourage contributions. I'll keep educating myself on that subject. Maybe having a contributor agreement like playcanvas could help. Thanks a ton for your feedback either way. I'll leave this issue open until I have a more definitive answer on this.

[kungfooman]

When it's with classes that do get inherited, using this.constructor gives this typescript error This expression is not constructable. Type 'Function' has no construct signatures.ts(2351)

Thank you for checking, this is a TypeScript issue that is open since 2015 with 201 thumbs-up: microsoft/TypeScript#3841

It is so important to write proper OOP but they simply don't care or don't know how to fix it. I would be happy if it finally works, but it may take another few years. 😅

Maybe it helps to simply raise awareness, the more people ask for it the better.

But it is restriction free otherwise.

That sounds good, when I read the Pricing page it said 0$ = Non Commercial only and all kinds of other restrictions. Maybe thats just the online editor, that is closed source anyway? Just from my personal experience it scares me and then I won't even try it.

Have fun brainstorming the licensing strategy, I'm glad my feedback was welcomed!

[frading]

Thank you for finding this typescript issue, very good to know. I'll see if some forced casting along with specific tests can improve this.

Regarding the license, yes, the free plan being limited to non-commercial only applies to the visual editor (which is indeed closed source). For the core library here, it's free to use, as long as it's not to compete with the editor and (soon to be released) marketplace. That should probably be clearer, so I'll have a think on how to improve that messaging.

[fire]

The license has been updated to plain MIT.

[kungfooman]

The license has been updated to plain MIT.

Hey @fire, thank you for the info! Commit: 6cd39d7