The Wayback Machine - https://web.archive.org/web/20210212110005/https://github.com/playcanvas/engine/pull/2641
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.

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

Transition to modern JavaScript #2641

Merged
merged 22 commits into from Dec 29, 2020
Merged

Transition to modern JavaScript #2641

merged 22 commits into from Dec 29, 2020

Conversation

@willeastcott
Copy link
Contributor

@willeastcott willeastcott commented Dec 26, 2020

Inspired by this talk from Chrome Dev Summit.

This PR migrates a portion of the PlayCanvas codebase from strict ES5 to support the very latest JS language features. Initially, the code has been updated to leverage:

  • Classes
  • Static class properties
  • Default parameters
  • const (sparingly for now)

Code updated so far is:

  • bundle
  • Most of core
  • math
  • resources
  • shape
  • template
  • extras (MiniStats)

This is all made possible by adding Babel to the engine's rollup build process.

The rollup configuration has also been updated to generate playcanvas.mjs, an ES Module build of the engine. So now we have:

  • Raw codebase: ES2021
  • playcanvas.mjs: Transpiled to ES6 (ECMAScript 2015)
  • playcanvas.js: Transpiled to ES5 (ECMAScript 2009)

This PR follows up on the previous unmerged PR that migrated the engine codebase to Typescript. There was much debate about the overall benefit of adopting Typescript. Typescript provides two main benefits:

  1. static type checking
  2. ability to adopt the latest JS language features

But we can achieve 2 with Babel. Therefore, it is probably easier to migrate to TS-like JS (ES6 classes in particular), and then revisit the benefits of Typescript. If Typescript is adopted at some point, this will be a stepping stone almost directly in that direction.

Let's look at output in terms of pc.Vec2 for example. Previously, it was:

	function Vec2(x, y) {
		if (x && x.length === 2) {
			this.x = x[0];
			this.y = x[1];
		} else {
			this.x = x || 0;
			this.y = y || 0;
		}
	}
	Object.assign(Vec2.prototype, {
		add: function (rhs) {
			this.x += rhs.x;
			this.y += rhs.y;
			return this;
		},

Now it's:

	var Vec2 = function () {
		function Vec2(x, y) {
			if (x && x.length === 2) {
				this.x = x[0];
				this.y = x[1];
			} else {
				this.x = x || 0;
				this.y = y || 0;
			}
		}

		var _proto = Vec2.prototype;

		_proto.add = function add(rhs) {
			this.x += rhs.x;
			this.y += rhs.y;
			return this;
		};

So essentially, a closure is introduced and Object.assign is removed. Still just as easy to debug.

Feedback most definitely welcome!

Closes #2642

I confirm I have signed the Contributor License Agreement.

@willeastcott willeastcott requested a review from playcanvas/reviewers Dec 26, 2020
@willeastcott willeastcott self-assigned this Dec 26, 2020
@willeastcott willeastcott changed the title Add Babel to build process Migrate from ES5 to ES6 (and beyond) Dec 27, 2020
@willeastcott willeastcott changed the title Migrate from ES5 to ES6 (and beyond) Transition to modern JavaScript Dec 27, 2020
@Maksims
Copy link
Contributor

@Maksims Maksims commented Dec 28, 2020

  • playcanvas.js: Transpiled to ES2015

I think this is mistake. As ES2015 (should be written as ECMAScript 2015), is ES6. And IE11 does not support ES6. It supports only ES5 (ECMA2009).

Tested few modern features, and babel seems to be weird with default parameters feature. It always (loose and not loose, with both targets) not outputing default parameters, while they are valid ES6, so should not be transpiled for ES6 target.

@Maksims
Copy link
Contributor

@Maksims Maksims commented Dec 28, 2020

I've tested more of Babel and Closure, what it outputs, and for ES5 both seem to do a good job, but for ES6, they start to differ.

Example 1:

Source:

class Ray {
    constructor(origin = new Vec3(0, 0, 0), direction = new Vec3(0, 0, -1)) {
        this.origin = origin;
        this.direction = Object.values(direction);
    }
}

Closure Compiler ES6:

class Qd {
    constructor(a=new z(0,0,0), b=new z(0,0,-1)) {
        this.origin = a;
        this.direction = Object.values(b)
    }
}

Babel + Terser ES6:

class Ray {
    constructor() {
        var e = arguments.length > 0 && void 0 !== arguments[0] ? arguments[0] : new Vec3(0,0,0)
          , t = arguments.length > 1 && void 0 !== arguments[1] ? arguments[1] : new Vec3(0,0,-1);
        this.origin = e,
        this.direction = Object.values(t)
    }
}

Issues:

  1. default parameters - are converted to ES5 format
  2. use of arguments leads to unoptimized (unable to compile to machine code) code in most JS engines.
  3. it seem to not mangling classes like Closure does, which leads to higher file size

Example 2:

Source:

containsPoint(point) {
    for (const plane of this.planes) {
        if (plane[0] * point.x + plane[1] * point.y + plane[2] * point.z + plane[3] <= 0) {
            return false;
        }
    }
    return true;
}

Closure Compiler ES6:

containsPoint(a) {
    for (const b of this.planes)
        if (0 >= b[0] * a.x + b[1] * a.y + b[2] * a.z + b[3])
            return !1;
    return !0
}

Babel + Terser ES6:

containsPoint(e) {
    for (var t of this.planes)
        if (t[0] * e.x + t[1] * e.y + t[2] * e.z + t[3] <= 0)
            return !1;
    return !0
}

Issues:

  1. It seem to not like let/const.

File sizes:

Right now, I've tested only on engine with small part moved to ES6+ code, but it already produces different file sizes:

  • Raw ES6+ + gzip: 387Kb
  • Closure Compiler ES5 + gzip: 297Kb (76.7%)
  • Closure Compiler ES6 + gzip: 294Kb (76.0%)
  • Babel + Terser ES5 + gzip: 287Kb (74.1%)
  • Babel + Terser ES6 + gzip: 303Kb (78.3%)

Size difference between them is insignificant. ES5 Babel + Terser is smaller than Closure Compiler ES5, for around ~2.5%. While ES6 version of Babel + Terser is bigger than Closure Compiler ES6 by around ~2.3%.

Size wise, both paths produce good results, but I assume that Closure Compiler will lead for ES6, as Babael + Terser sometimes transpiles valid ES6 code to ES5.

My only concern, is why Babel + Terser is transpiling some of valid ES6 features into ES5, when instructed to use ES6 target.

@willeastcott
Copy link
Contributor Author

@willeastcott willeastcott commented Dec 28, 2020

@Maksims Ha, yes, sorry, I get confused with all the JavaScript language naming. You're right - I have corrected my original post.

Issues:
default parameters - are converted to ES5 format

Fixed with this commit.

use of arguments leads to unoptimized (unable to compile to machine code) code in most JS engines.

I'm not seeing this. With the commit above, I'm getting the following (after beautifying):

class Ray {
    constructor(e = new Vec3, t = new Vec3(0, 0, -1)) {
        this.origin = e, this.direction = t
    }
    set(e, t) {
        return this.origin.copy(e), this.direction.copy(t), this
    }
}

it seem to not mangling classes like Closure does, which leads to higher file size

Yeah, I noticed that. I'm not sure why but I don't think it's making a significant difference.

@willeastcott
Copy link
Contributor Author

@willeastcott willeastcott commented Dec 28, 2020

Oh, and that let + const thing is also fixed by that commit.

src/core/color.js Outdated Show resolved Hide resolved
@@ -4,6 +4,11 @@ import { Vec3 } from './vec3.js';
import { Vec4 } from './vec4.js';

var _halfSize = new Vec2();
var x = new Vec3();

This comment has been minimized.

@mvaligursky

mvaligursky Dec 29, 2020
Contributor

just curious if these could be inside class now as static or similar?

This comment has been minimized.

@mvaligursky

mvaligursky Dec 29, 2020
Contributor

also, will these variables with names "x" "y" "z" create possible conflicts in the rest of the class?

This comment has been minimized.

@willeastcott

willeastcott Dec 29, 2020
Author Contributor

We could do. I think I'd probably put them on the Vec3 class as Vec3._temp1, Vec3._temp2, Vec3._temp3 maybe. But I personally don't have a big problem with these being global to the module. It's kind of nice because the app code cannot access them (and therefore misuse them), which isn't true of they're on the class.

This comment has been minimized.

@willeastcott

willeastcott Dec 29, 2020
Author Contributor

And no, conflicts aren't possible. Rollup renames variables that conflict when building the bundled library. It's pretty cool!

This comment has been minimized.

@mvaligursky

mvaligursky Dec 29, 2020
Contributor

I mean more like conflict with a local variable inside function of Vec3. Few functions have "var x". If I don't have this var line would x reference the global x?

This comment has been minimized.

@willeastcott

willeastcott Dec 29, 2020
Author Contributor

Yes.

src/math/vec2.js Outdated Show resolved Hide resolved
src/math/vec3.js Outdated Show resolved Hide resolved
src/math/vec4.js Outdated Show resolved Hide resolved
this.center.copy(src.center);
this.halfExtents.copy(src.halfExtents);
this.type = src.type;

This comment has been minimized.

@mvaligursky

mvaligursky Dec 29, 2020
Contributor

should be ok to remove, I don't see references to it .. but curious what it was?

This comment has been minimized.

@willeastcott

willeastcott Dec 29, 2020
Author Contributor

So this used to be a string type property on each shape instance that specified what it was. But it was removed years ago. This bug was actually pointed out to me by the Typescript compiler but since that PR is unlikely to be merged, I'm adding the fix here.

src/shape/oriented-box.js Outdated Show resolved Hide resolved
Copy link
Contributor

@mvaligursky mvaligursky left a comment

looking great!

@mvaligursky

This comment has been minimized.

Copy link
Contributor

@mvaligursky mvaligursky commented on .github/CONTRIBUTING.md in 684021f Dec 29, 2020

should use "const" instead of "let"?

This comment has been minimized.

Copy link
Contributor Author

@willeastcott willeastcott replied Dec 29, 2020

Fixed

@willeastcott willeastcott merged commit bd92b6d into master Dec 29, 2020
1 check passed
1 check passed
build
Details
@willeastcott willeastcott deleted the babel branch Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants