Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Clone methods do not respect proper OOP #50

Open
kungfooman opened this issue Aug 9, 2022 · 7 comments
Open

Clone methods do not respect proper OOP #50

kungfooman opened this issue Aug 9, 2022 · 7 comments

Comments

@kungfooman
Copy link

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
Copy link
Collaborator

frading commented Aug 9, 2022

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
Copy link
Author

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
Copy link
Collaborator

frading commented Aug 9, 2022

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
Copy link
Author

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
Copy link
Collaborator

frading commented Aug 10, 2022

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
Copy link

fire commented Oct 17, 2023

The license has been updated to plain MIT.

@kungfooman
Copy link
Author

The license has been updated to plain MIT.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@fire @frading @kungfooman