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

Add support for ES6 script classes #1908

Merged

Conversation

aidinabedi
Copy link
Contributor

@aidinabedi aidinabedi commented Mar 8, 2020

Fixes #1907 (also #1889 and #1887).

PR consists of the following significant changes:


Using the PR devs can now choose between either regular ES5-style or ES6-class scripts:

ES5 example:

var CameraController = pc.createScript('CameraController');

CameraController.attributes.add('myAttrib', { type: 'boolean', default: false });

CameraController.prototype.initialize = function() {
    // initialize code called once per entity 
};

CameraController.prototype.update = function(dt) {
    // update code called every frame
};

ES6 example:

class CameraController extends pc.ScriptType {

    initialize() {
        // initialize code called once per entity 
    }

    update(dt) {
        // update code called every frame
    }
}

CameraController.attributes.add('attribute', { type: 'boolean', default: false });

pc.registerScript(CameraController);

All changes have been tested using a public PlayCanvas project (simply run it with and without the PR):

With PR:

image

Without PR:

image

I confirm I have signed the Contributor License Agreement.

@willeastcott
Copy link
Contributor

This looks pretty cool. @vkalpias and @Maksims - any objections before I merge?

@willeastcott willeastcott self-requested a review March 9, 2020 23:13
@Maksims
Copy link
Collaborator

Maksims commented Mar 10, 2020

I'm good, only have few things to be clarified before merging:

  1. From first glance I don't see any behavioral changes, and to keep this PR simple its good. If I'm wrong, please clarify.
  2. This PR needs to be thoroughly tested on existing projects Editor based as well as engine-only.
  3. With introduction of ES6 Classes path, we should update Editor WebWorker for parsing attributes. It should still be optional path, so not to drop support of IE11 users of Editor.
  4. If developer decides to use ES6 Classes path in Editor, this makes Editor not usable in Internet Explorer 11. Glance at analytics will probably indicate very tiny fraction of users who it potentially affects.
  5. This slightly affects performance, due to extra prototype level on script objects. But perhaps this would be insignificant to even measure the impact.

@willeastcott willeastcott self-assigned this Mar 10, 2020
@willeastcott
Copy link
Contributor

@Maksims

  1. Yeah, I think that's correct.
  2. Yep, we'll run the full test plan before we deploy this.
  3. The Editor doesn't support IE11 since last October or something like that (when we switched to laying out everything with flexbox).
  4. See 3.
  5. I suspect you're right that the performance implications are negligible.

@Glidias
Copy link
Contributor

Glidias commented Mar 23, 2020

Some things to take note of:

  1. Is Class<pc.ScriptType> a proper type in TypeScript? I tried out that type signature in the latest typescript on playground and it yields an error. Typescript definitions might cause an error with this as well. This won't solve the problem i have with regards to type hinting/intellisense ( a bare minimum for me), and possibly even workable ts-checking of .js files (good to have, though not critical). If not using the editor IDE on browser, I'm using Visual studio code.

  2. For pc.registerScript, if you don't supply an explicit script name, and you are using minifiers (particularly along build tool chains such as webpack), there can be cases where the __getFuncName() method might end up referencing mangled class names typically by default. (THe same cases may occur with constructor.name cases...) This can/may be circumvented with minifier options to preserve function names, but i can't rmb exactly the specific edge cases where even the option to preserve function names might still not work.

I can't test custom engine with editor IDE in browser to actually see how the ES6 intellisense thing behaves . But from the code itself, since it extends the ScriptType class, it shouldn't have any intellisense issues with regards to that. Just got to remember to call "registerScript" at the end (or use a decorator for that) to register the script into playcanvas.

@aidinabedi
Copy link
Contributor Author

@Glidias Thank you for the comment.

  1. Class<T> is a tsd-jsdoc convention that is automatically converted to typeof T. So if you generate the TypeScript definitions you will see that the actual TypeScript code results in typeof T instead of Class<T>. Similarity the API reference documentation will also automatically be converted to typeof T when the new version is officially released.

  2. Correct, the __getFuncName() will end up returning mangled class names, if minifed. That is why pc.registerScript() has an optional name parameter which will let you specific the exact class name for precisely such cases. Would probably be good to add that in the documentation (in a separate PR). Your more than welcome to contribute! :)

Agreed. Ideally, there would be a pc.scriptType decorator which would take care of calling registerScript. That is something I would love to add later as a built-in feature, given the opportunity.

@Glidias
Copy link
Contributor

Glidias commented Mar 24, 2020

Decorators though would require custom babel settings and such since it's an experimental feature , so I don't think it can be a built in feature even though technically the method could still be included as a seperate module file (or then again, it's possibly just a one-liner importable decorator method anyway) for optional use.

On a sidenote, I have some fancy @attrib decorators I used in some of my babel/webpack integration examples for ES6 (mainly TypeScript) classes (you can check my repos). But again, all these are just syntax sugar that can only work in certain build/config environments. (Not forgetting that there is a certain functional call overhead, though likely negligible, when using such decorators).

@Glidias
Copy link
Contributor

Glidias commented Mar 24, 2020

  1. Yes, it does export out to typeof T. I tested it out and looks like the prototype implementation methods now provide typehinting, but the problem though, is now you lose typehinting/type-checking for .attributes property on the typeof T (=>Class) itself. Shouldn't readonly attributes be static readonly attributes instead?
ES5Script.prototype.initialize = function() {
    this.fire('initialize');
    alert(this.attributes);
};

attributes is alerted as undefined. So, there is no case whatsoever in Playcanvas where "attributes" would also exist within the instance itself. This goes to show that attributes should had simply been a static property all this while. Since T = pc.createScript() now returns typeof T in the latest test, setting attributes to static will reinstate type-hinting back for ScriptTypeClass.attributes.

Weird though, because you did include @static in the jsdocs:

 /**
 * @field
 * @static
 * @readonly
 * @name pc.ScriptType#attributes
 * @type {pc.ScriptAttributes}
 * @description The interface to define attributes for Script Types. Refer to {@link pc.ScriptAttributes}.
 * @example

But somehow, using @static only without explicit static readonly declaration, doesn't seem to square off intellisense for me.... ...... (attributes should be availalble on the squiggie reds, and .attributes should not be found within the prototype handlers),
image

@aidinabedi
Copy link
Contributor Author

aidinabedi commented Mar 29, 2020

@Glidias I've fixed the issue with attributes not being recognized as static. It was simply due to an old incorrect naming syntax (pc.ScriptType#attributes -> pc.ScriptType.attributes).

TheJonRobinson added a commit to Mojiworks/playcanvas-engine that referenced this pull request Mar 30, 2020
* stable: (48 commits)
  [RELEASE] v1.26.0
  [FIX] null access when pushing SpectorJS marker without camera (playcanvas#1933)
  [FIX] pc.XrHitTest when WebXR not available
  Update package-lock.json
  Downgrade mocha to 6.2.2
  WebXR AR hit test support (playcanvas#1926)
  [ImgBot] Optimize images (playcanvas#1927)
  [DOCS] Make anim API private (playcanvas#1924)
  Update NPM dependencies.
  [FIX] Update some example paths
  Update to latest JSDoc template
  [FIX] Updating of particle systems local bounds
  Add pc.drawTexture to render texture to target using a shader (playcanvas#1920)
  Optimize Grab-Pass for WebGL 2 and fix when using anti-aliased RenderTarget (playcanvas#1918)
  [DOCS] Document drawQuadWithShader and copyRenderTarget (playcanvas#1919)
  [FIX] Mouse wheel events on elements (playcanvas#1916)
  glTF 2.0 support (playcanvas#1904)
  Update to latest JSDoc template.
  Add support for ES6 script classes (playcanvas#1908)
  Particle system randomize sprite animations (playcanvas#1911)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ES6 script classes
4 participants