mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
102.81k stars 35.38k forks source link

Evaluate ES6 classes #11552

Closed arctwelve closed 3 years ago

arctwelve commented 7 years ago

Why is this 'idiom' of inheritance being introduced into the code:

PointLight.prototype = Object.assign( Object.create( Light.prototype ), {

Seriously? Function(nested Function(ParentPrototype) COMMA SHOTGUN BRACKET?

The faithful two line style still in, for example, the Materials classes are much clearer and cleaner. Assign the prototype and then set the constructor. The end. Please don't ruin the library by catching JavaScript disease - the bizarre need to masturbate the way objects and inheritance are coded. One style throughout the library. No need to change it.

mrdoob commented 7 years ago

So you're suggesting using this pattern instead?

PointLight.prototype = Object.create( Light.prototype );
Object.assign( PointLight.prototype, {
sasha240100 commented 7 years ago
class PointLight extends Light

hehe 😄 And no problems...

looeee commented 7 years ago

@sasha240100 someday...

looeee commented 7 years ago

@mrdoob not quite - the two ways you mention are directly equivalent. I think the OP is comparing

PointLight.prototype = Object.assign( Object.create( Light.prototype ), { 
    constructor: PointLight,
    prop1: 'something',
    method1: function someFunction() { .. },
    ...
});

with

function PointLight () { ... };

PointLight.prototype = Object.create( Light.prototype );

PointLight.prototype.constructor = PointLight;

PointLight.prototype.prop1 = 'something';

PointLight.prototype.method1 = function someFunction() { .. };

...

Which is the way it's done here for example. As far as I can see these styles are equivalent - is there something I'm missing? Or was the style changed to use Object.Assign once it became available and not updated across the codebase?

sasha240100 commented 7 years ago

@looeee @bfred-it @mrdoob Why not just using rollup-babel ?

Comparison: Current. es5 + es6 harmony modules.

import { LineBasicMaterial } from './LineBasicMaterial';
import { Color } from '../math/Color';

function LineDashedMaterial( parameters ) {

    LineBasicMaterial.call( this );

    this.type = 'LineDashedMaterial';

    this.scale = 1;
    this.dashSize = 3;
    this.gapSize = 1;

    this.setValues( parameters );

}

LineDashedMaterial.prototype = Object.create( LineBasicMaterial.prototype );
LineDashedMaterial.prototype.constructor = LineDashedMaterial;

LineDashedMaterial.prototype.isLineDashedMaterial = true;

LineDashedMaterial.prototype.copy = function ( source ) {

    LineBasicMaterial.prototype.copy.call( this, source );

    this.scale = source.scale;
    this.dashSize = source.dashSize;
    this.gapSize = source.gapSize;

    return this;

};

export { LineDashedMaterial };

ES2015+. Same code, but es2015+ with babel-plugin-transform-class-properties:

import { LineBasicMaterial } from './LineBasicMaterial';
import { Color } from '../math/Color';

export class LineDashedMaterial extends LineBasicMaterial {
  type = 'LineDashedMaterial';

  scale = 1;
  dashSize = 3;
  gapSize = 1;
  isLineDashedMaterial = true;

  constructor(parameters) {
    super();
    this.setValues( parameters );
  }

  copy(source) {
    super.copy(source);

    this.scale = source.scale;
    this.dashSize = source.dashSize;
    this.gapSize = source.gapSize;

    return this;
  }
}

ES6 features that would simplify three.js code:

mrdoob commented 7 years ago

I'm all for moving to ES2015+, we just need to find a way to output similar code than what we currently have out of it, so the performance stays the same in all cases.

Mugen87 commented 7 years ago

I have a question in context of classes. How would we transfer methods like Vector3.unproject into the class syntax? The method actually uses a closure in order to create a new scope. This is an important mechanism that keeps the amount of object creations as low as possible.

Do we need Object.assign in these cases?

sasha240100 commented 7 years ago

@Mugen87 @mrdoob Some interesting info on es6 performance. Especially on Object.assign: image From this article

satori99 commented 7 years ago

How would we transfer methods like Vector3.unproject into the class syntax? The method actually uses a closure in order to create a new scope.

@Mugen87 Can't they just be non-exported module scoped objects? Something like this;

const tempMatrix = new Matrix();    

export default class Vector3{
    unproject() {
        // uses tempMatrix
    }
}
Mugen87 commented 7 years ago

Ah yes, i think this should work 😊

mrdoob commented 7 years ago

Last month this was suggested on twitter:

https://astexplorer.net/#/gist/9db8b707e3f69f94ded4af737f879fa7/b12fb12806e97df96278ec8b7d52018c8421872f

sasha240100 commented 7 years ago

@mrdoob Wow! It seems already working. Can't we make a branch, transform some classes to es6 and see how it compiles?


@satori99 As an idea of how to keep tempMatrix inside Vector3 code to avoid problems with globals:

export default class Vector3 {
    static tempMatrix = new Matrix();

    unproject() {
        // uses Vector3.tempMatrix
    }
}
mrdoob commented 7 years ago

@mrdoob Wow! It seems already working. Can't we make a branch, transform some classes to es6 and see how it compiles?

Sound good to me! I'm currently focusing on WebVR so it'll need to be someone else than me.

satori99 commented 7 years ago

@sasha240100 The benefit of using module scoped vars is that they remain hidden from regular user code, which seems appropriate for temp variables.

I don't mind the "We are all adults here" pythonistic approach in regards to private vars, but temp variables shouldn't really pollute the namespace unnecessarily.

Also, It would be nice if those of us who have enabled native module support in our browsers could load the src files directly. This is a much more pleasant way to develop, without needing watchers and transpiling after every edit. Using class properties means this isn't possible as they are not part of the current class spec.

agnivade commented 7 years ago

Apologies for butting in. We should probably change the issue title to something like - "Evaluate ES6 classes", since now the thread has changed to something completely different.

pailhead commented 7 years ago

Why change the pattern @Mugen87 @satori99 ?

method =(()=>{ 
    const vec3forThisScope =...; 
    return (arg)=>{...}
})()
FishOrBear commented 7 years ago

Why not try typescript, it can be compiled into other versions of js

joejordanbrown commented 7 years ago

TypeScript really would be a great option to think about because it's a transpiler + type checker and a superset of JavaScript, so it's easy to move a code base over to .ts files and gradually refactor to ES6 with type checking.

It may sound scary if you've never used TypeScript, but it's really not a big learning curve and would be a small price to pay for the benefits it would bring. The TypeScript community would be very happy to help with this transition and creating performance testing against the current library to make sure it's not being downgraded.

A few useful articles:

To quote of core developer Anders Hejlsberg, TypeScript was born in response to complaints from clients and internal teams that JavaScript didn't lend itself well to large applications.

The goal was to "strengthen JavaScript with things like classes, modules, and static typing", without sacrificing the advantage of its being open-standards and cross-platform; the result was a "language for application scale javascript development", built as a superset of the language.

mrdoob commented 7 years ago

Until browsers can execute natively TypeScript, I prefer to keep using JavaScript.

joejordanbrown commented 7 years ago

@mrdoob

I can't see that as a valid reason for not using TypeScript solely on the reason it can't be run directly in the browser. You wouldn't want it to run in the browser because of all the extra lines of code that is intended only for compile time checking. It's not currently a runtime checking language. So if it was ever used in the browser it's more than likely going to have all the typed code stripped away because it impacts performance and this would then be vanilla JavaScript code.

I think you're totally missing the point of using a typed language and the benefits it has in development in a large code base. You are still writing and using JavaScript, the whole point of TypeScript is that it is a superset of JavaScript. You write JavaScript with types, that's compiled into JavaScript in the specified ECMAScript target version, which is configurable in the compiler options, the permitted values are 'es3', 'es5', 'es2015', 'es2016', 'es2017' or 'esnext'.

Because Typescript is JavaScript it makes it possible to progressively migrate without having a massive headache of refactoring everything at once. It can be gradually done and improved by the community. It's no more work than what's being discussed here with refactoring to use ES6 classes. That's the only reason I mention it here instead of opening a new issue.

See TypeScript playground links below for great examples:

pailhead commented 7 years ago

@joejordanbrown

Can you think of anyone that could possibly disagree with you about typescript being the best solution to this particular problem?

If you type in typescript vs into google, a few terms pop up, one of them being Flow. Searching for that, seems to yield a number of articles where people are debating the pros and cons of these two.

No types seems like more of a compromise than choosing one of these.

arctwelve commented 7 years ago

Save Typescript for projects that are more complicated than the result they create -- specially frontend frameworks that could have been implemented in HTML in the first place. My original point was to get rid of the JavaScript disease, not make it worse. JavaScript is a simple almost toy language that sometimes is used for complex results like three.js. Typescript is pointless.

On Sep 6, 2017, at 1:55 PM, Joe notifications@github.com wrote:

@mrdoob

I can't see that as a valid reason for not using TypeScript solely on the reason it can't be run directly in the browser. You wouldn't want it to run in the browser because of all the extra lines of code that is intended only for compile time checking. It's not currently a runtime checking language. So if it was ever used in the browser it's more than likely going to have all the typed code stripped away because it impacts performance and this would then be vanilla JavaScript code.

I think you're totally missing the point of using a typed language and the benefits it has in development in a large code base. You are still writing and using JavaScript, the whole point of TypeScript is that it is a superset of JavaScript. You write JavaScript with types, that's compiled into JavaScript in the specified ECMAScript target version, which is configurable in the compiler options, the permitted values are 'es3', 'es5', 'es2015', 'es2016', 'es2017' or 'esnext'.

Because Typescript is JavaScript it makes it possible to progressively migrate without having a massive headache of refactoring everything at once. It can be gradually done and improved by the community. It's no more work than what's being discussed here with refactoring to use ES6 classes. That's the only reason I mention it here instead of opening a new issue.

See TypeScript playground links for examples:

Classic JavaScript Example Adding Types Example Adding Types with error Example Using Classes Example Using Classes with error Example — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

pailhead commented 7 years ago

^ i'd be fine even with the monstrous pattern, as long as it's consistent.

mrdoob commented 7 years ago

@joejordanbrown sounds like you're in love with typescript. feel free to fork the project and port it to typescript. three.ts! 🙌

joejordanbrown commented 7 years ago

@pailhead

That's a matter of choice, I'm sure many will agree and disagree, that's normal though right! You're always going to see "this vs that", "mine is better than yours". I understand that each has their own benefits. It's about weighing up the options available and seeing if they can benefit the project. Comparisons are a good thing, it pushes projects further.

You mention Flow, the problems I see with that are:

Even google is backing TypeScript in a big way, that shows the confidence they have in TypeScript. Read the post here or here.

Typescript has become allowed for unrestricted client development as of March 2017. TypeScript and Angular on TypeScript are used in Google Analytics, Firebase, and Google Cloud Platform and critical internal tools such as bug tracking, employee reviews, and product approval and launch tools.

I just thought I'd open the discussion about using a Typed language and see what everyone else thinks about the idea. It does seem that @mrdoob is totally against even discussing the idea though.


@arctwelve I don't see how this project isn't complicated and how using a Typed language would affect it negatively.


@mrdoob Not at all, I can just see the benefits it could have especially if a new branch is being created to update to ES6 classes. I think to answer with create your own fork called three.ts is just being silly. That's really against good OSS practices if everyone just forked OSS projects and modified their own source code instead of focusing on the 1 project and making it the best it can be. You would end up with really poor software or communities splitting and focusing on the project they preferer for whatever reason. Why can't you have an open discussion about the pros and cons?

pailhead commented 7 years ago

Not to play the devils advocate, but it seems like he did

have an open discussion

it was just really short :)

I share a similar standpoint, it's a JS library and JS is standardized. You can't go wrong with choosing JS for a JS library, while you can if you choose something else. I just took Flow as one of the alternatives to typescript, dunno if there are others.

Anyway, it seems like we really went off topic.

Mugen87 changed the title from Remove JavaScript Disease to Evaluate ES6 classes

The original title referred to (as far as i understand) to the lack of style consistency. In particular using Object.assign() in some places, and another pattern in others. If i'd evaluate anything here, it would be the current title of the issue. Why is the issue of consistency being elevated to a discussion about using a new language?

I imagine that with both typescript and es6, the code should be fairly consistent.

I'd address this issue by updating this page:

https://github.com/mrdoob/three.js/wiki/Mr.doob's-Code-Style%E2%84%A2

and adding either:

A) "...use Object.assign ..." B) "...dont use Object.assign"

pailhead commented 7 years ago

One style throughout the library. No need to change it.

The faithful two line style still in, for example, the Materials classes are much clearer and cleaner.

It's in the first post.

I suggest:

  1. edit the title to reflect this sentence, discuss having one style throughout the library, editing the style guide etc.
  2. start a new discussion titled "evaluate es6 classes", where es6 classes would be evaluated
  3. start a new discussion titled "evaluate having three written in a typed language", where typescript and such would be discussed
mrdoob commented 7 years ago

Anyway, it seems like we really went off topic.

Indeed. @joejordanbrown feel free to create a new topic to discuss TypeScript.

Btw, it's also bad OSS practice to ignore previous conversations... https://github.com/mrdoob/three.js/issues/341#issuecomment-47000692

mrdoob commented 7 years ago

Back to the topic. I thought we already solved this issue?

https://github.com/mrdoob/three.js/issues/11552#issuecomment-319449068

We just need someone to give it a try.

Itee commented 7 years ago

Ok so... first of all

First pattern ( the best IMO ):

function MyClass() {...}

MyClass.prototype = Object.assign( Object.create( MyClassToInherit.prototype ), {

    constructor: MyClass,

    prop1: 'something',

    method1: function someFunction() { .. },

    ...

});

The second pattern:

function MyClass() {...}

MyClass.prototype = Object.create( MyClassToInherit.prototype );

MyClass.prototype.constructor = PointLight;

MyClass.prototype.prop1 = 'something';

MyClass.prototype.method1 = function someFunction() { .. };

...

@arctwelve This pattern was introduce for many reason. This is not masturbate !

First of all it allow a clear reading about the object inheritance. The Object.assign is clearly here about the object inheritance. Then you can't lose the inherited object in many and many line of MyClass.prototype. Second, in case of multiple inheritance, this is much clearer too. Third, the constructor of the class is readable and is not lose in many line like the first point. Fourth, this allow to group properties and methods in the same location ( inside bracket ) which is very much clearer when you have 3, 4, 5... etc classes in the same file. Fifth, this pattern allow to check correct copy for some "inherited" properties.

Finally ( @looeee ), this is for performance too. The first version is more optimized when file parsing occur, instead of multiple and multiple call to prototype.

Any way !

Nowaday, we should pass to ES6 syntax !

@mrdoob

Sound good to me! I'm currently focusing on WebVR so it'll need to be someone else than me. We just need someone to give it a try.

Would you create an es6 branch ? Or it's on our own ?

looeee commented 7 years ago

The first version is more optimized when file parsing occur, instead of multiple and multiple call to prototype.

Are you sure about that? I'd expect Object.Assign would be slower, if anything. But in either case I doubt it's enough of a performance overhead to worry about.

Itee commented 7 years ago

Absolutely: https://jsperf.com/inline-prototype-vs-assign-prototype/1

Under chrome version 61.0.3163.100 (Build officiel) (64 bits) Assigned prototype are around 60% faster

looeee commented 7 years ago

Interesting. Thanks for making the test.

That result doesn't hold up for all browsers though. On Firefox they are nearly the same (Object.Assign ~3% faster), while on Edge Object.Assign is ~33% slower.

But in any case I still don't think this is relevant as an argument for which style is better - even the slowest overall (inline proto in Chrome) is still running at >180,000 operations a second, and there's maybe a couple of thousand of these setups done in the code. So we are talking about a difference of a few milliseconds here, probably.

For me, the Object.Assign style is cleaner and easier to read, and that's the main argument in favour of it.

Itee commented 7 years ago

Yes this is about few miliseconds per file and only when file is parsed the first time by javascript engine... but for a big library like threejs the gain could be reasonably around 200 miliseconds on page load. Do not forgot the limit of 3scd for front user that aren't able to wait more !

FriOne commented 6 years ago

I'm trying to make Angular project with threejs and it always looks like I'm hacking every part of threejs. First of all it is es5 syntax with THREE constant that must exist, for example, if I need OrbitalControls. We have threejs typings, but it will be more convenient to have them in the same package. The typings have OrbitalControls, but we can't simply import like import { OrbitalControls } from 'three;. Webpack have tree shaking, so in case of es6 we could include all we need inside one project and not to move them to separated one. @mrdoob so why Typescript is so bad? It will be compiled to ES any version with typings anyway. It is also used in many other frameworks like React.

tschoartschi commented 6 years ago

@FriOne I'm not sure if @mrdoob really thinks Typescript is bad but I think he is against discussing Typescript in this issue/thread because it's not the topic of this thread. I think ES6 classes don't work against or for Typescript. If you transform the codebase to ES6 it's even easier to port the codebase over to Typescript because it's syntax is very similar. Yes I think Typescript could enable a lot of new options. For example, it could help to "transpile" a more optimized code, it helps new developers to learn the library faster, maybe it opens doors for experiments in the future e.g.: https://github.com/AssemblyScript/assemblyscript. I think there are a lot of advantages with Typescript @joejordanbrown described the pros in detail.

But back to topic: I think adopting ES6 classes would be a step forward and after that, we could discuss the Typescript thing. So let's do one step after each other

FriOne commented 6 years ago

@tschoartschi, think typescript can help with migration to es6 classes and other refactoring. I don't have such migration experience, it might be I'm wrong.

tschoartschi commented 6 years ago

@FriOne it depends 😉 of course Typescript could help because the compiler could tell you all your errors and mistakes but you would need to set up the whole build pipe first to work with Typescript. Furthermore, you need to assess if Typescript is the correct fit or not. I think it's fine to convert to ES6 classes first and then think about Typescript.

hbogaeus commented 6 years ago

Hey, we're a group of 5 students from KTH looking to contribute to an open source project for a course and would like to try and convert a part of the project to the new ES6 syntax.

flyover commented 6 years ago

I ported Three.js to TypeScript a while back (r82) along with a few of the examples, as a proof-of-concept.

https://github.com/flyover/three.ts

The examples take a bit to load, as the TypeScript source is transpiled on the fly. They load as fast as the originals when using the transpiled JavaScript.

pkieltyka commented 5 years ago

I'd love to see three.js ported to typescript. I feel the project badly needs to be modernized if it wants to stand the test of time for the next web generations. One day, we may even see it working with AssemblyScript and running in WASM. If not threejs, then something else surely will.

bhouston commented 5 years ago

This is completed @WestLangley and @mrdoob

arodic commented 5 years ago

@bhouston what is the conclusion here?

tschoartschi commented 5 years ago

@pkieltyka yes I also think TypeScript would make a lot of sense for a library like Three.js. Beside all the technical pros it would also ease the use of the library and helps newbies to explore the API. But I also think it's important to finish all the ES6 Stuff first and then consider TypeScript. I think from the latest JavaScript it's not too complicated to add types in form of TypeScript.

@flyover nice too see a proof-of-concept for a TypeScript version of Three.js. Did you got feedback from the maintainers of Three.js?

bhouston commented 5 years ago

I also support typescript for three.js. typescirpt is basically best practice and raw JavaScript isn't any more.

On Sat, Jan 5, 2019, 4:13 AM tschoartschi <notifications@github.com wrote:

@pkieltyka https://github.com/pkieltyka yes I also think TypeScript would make a lot of sense for a library like Three.js. Beside all the technical pros it would also ease the use of the library and helps newbies to explore the API. But I also think it's important to finish all the ES6 Stuff first and then consider TypeScript. I think from the latest JavaScript it's not too complicated to add types in form of TypeScript.

@flyover https://github.com/flyover nice too see a proof-of-concept for a TypeScript version of Three.js. Did you got feedback from the maintainers of Three.js?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mrdoob/three.js/issues/11552#issuecomment-451639995, or mute the thread https://github.com/notifications/unsubscribe-auth/AAj6_bkdND7I0_F4AJcBV0DYLpToUIVhks5vAGykgaJpZM4N9vH8 .

roomle-build commented 5 years ago

@bhouston I agree that TypeScript is a great technology but I wouldn't put it the way you did. I think we should always stay as close as possible to raw JavaScript and add the TypeScript features on top. Since TypeScript follows the JavaScript spec very closely, TypeScript really reads like ES6 with types.

For a library like three.js TypeScript could be really beneficial and could be adopted gradually. So we wouldn't need a "big-bang" rewrite, especially not after all the ES6 refactors are finished.

Would be interesting to hear if @mrdoob 's stance on TypeScript changed. TypeScript seems to become the "defacto" standard for "typed JavaScript" (note that I put my claims under apostrophe since this are no hard facts)

looeee commented 5 years ago

The first step should be adopting ES6 features, especially classes, arrow function, 'let', and 'const'.

Once we've done that we can properly discuss typescript support since, as @roomle-build points out, it's easy to incrementally add typescript features on top of ES6 code gradually if we decide to.

Doing both at once seems like it would over-complicate things, to me.

roomle-build commented 5 years ago

Great to hear that TypeScript could be an option at some point in the future :-) maybe we could reuse some of the work done by @flyover

But I totally agree with @looeee to finish all the ES6 stuff first and then focus on the next steps

looeee commented 5 years ago

finish all the ES6 stuff

I'd be happy if we could at least start it 😅

bhouston commented 5 years ago

A good half way step to TypeScript woudl be to add type files beside every JavaScript file. Thus there would be both:

Vector3.js Vector3.d.ts

This gives us all the benefits of TypeScript as a sidecar files.

Right now there is a @types/three file but it is out of date and maintained separately -- thus it will be always out of data.

The main competitor to Three.JS is Babylong and it is fully typescript and I believe it benefits from this.

But killing @types/three and integrated it as side cars type definition files into Three proper would be a great first step.