playcanvas/engine

Spineboy engine example fails to work when using ES6 build

mvaligursky opened this issue · 37 comments

I don't see the codebase calling https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/preventExtensions and so I'm not sure what is causing this yet.

if I build the examples this way, it works:
engine: target=debug_es6 npm run watch
examples: ENGINE_PATH=../build/playcanvas.dbg.mjs/index.js npm run develop

but when I use npm run build for both, it does not

@ellthompson - any suggestions here?

Similar issue takes place when using module build on examples that use post-effect, for example area-picker. See the selected line causing the issue, again related to access to pc. namespace

image

See the selected line causing the issue, again related to access to pc. namespace

It's because you cannot mix ES5 and ES6... i mean you can, but then you run into issues like that.

So the choice is all-in or all-out:

Works:

function Animal() {}
function Bird(fName, lName, eId) {
  Animal.call(this);
}
bird = new Bird()

Doesn't work (because mixing ES5 and ES6):

class Animal {}
function Bird(fName, lName, eId) {
  Animal.call(this);
}
bird = new Bird()

image

Personally I don't waste my time with these kinds of issues any longer and I'm simply all-in for ES6.

Neither do I want or need any bundler during the development process, so I rewrote the example system into ES6 for myself to remove all the *.tsx sugar into pure *.mjs.

Why should we still cling to ES5? We already assume everyone uses WebGPU but ES5 is backwards-compatibility? I don't know how that works together.

Obviously we still want to ship the engine in ES5, but transpiling into ES5 is exactly what bundlers excel at.

@kungfooman - Thanks for the response here. We understand and agree to need to move to ES6, and we're working on a solution for this as an example: #4767

But it's non-trivial to move there - this is one of the larger things we need to handle in order to move fully to ES6.

Thank you for the response too.

How does that issue block anything? You simply need to rewrite it into ES6:

image

The old entity script style works in ES6 without problems. The only issue here is trying to extend an ES6 class in the ES5 way.

But this change makes the script to be ES6 and it no longer works when using ES5 only browsers. We could maintain both versions / transpile it to ES6 or similar, but this change is not all there is unfortunately.

So the options are:

  1. Using a tool like https://github.com/wessberg/cjstoesm and make the build process of /examples/ even more complicated to build an ES5 and ES6 version, fetching ES5/ES6 versions at either build- or runtime.

  2. Accept that /examples/ is pretty much ES5 and if you try ES6, every simple ES5 script may crash it

  3. Rewrite everything into ES6 and tell users to use bundlers/transpilers for backwards-compatibility

  4. Have *.js and *.mjs files next to it (functionally identical) - this may also make (1) easier

Another ES6 issue in Spine:

image

Object.defineProperty(Spine.prototype, "state", {
	get: function () {
		return this.states[0];
	}
});

(because we only have a getter without setter)

If we wanted to have proper ES6, we would need to rewrite https://github.com/playcanvas/playcanvas-spine or just fix bugs on a 1-on-1 basis.

At least I believe that's easiest (transpiling down to ES5)

Note that this issues only take place on the ES6 build of the engine / examples. Using ES5 for both works great.

There's a few steps in correcting this gracefully. First question:

Before it is transpiled, what is the language level are developers expected to write and submit code in and why es2022 ? ;-)

We can use this answer to add one line into the eslint configuration to warn when developers go above it. All pull requests will be checked against it so it will keep the code base clean.

@playcanvas/eslint-config is currently setting the following:

  "parserOptions": {
    "ecmaVersion": "latest",  // configures tools 2023 at the moment
    "sourceType": "module",
    ...
  },

  "env": {
    "browser": true,
    "es2020": true,  // sets environment for 2020
    "node": true,
    "worker": true
  }

I would suggest feeding this back into the @playcanvas/eslint-config setting matching values, to keep tooling consistent.

There's a few steps in correcting this gracefully. First question:

Future PR's, yes, for commited code, no:

https://github.com/playcanvas/playcanvas-spine/blob/71ec3c5d9a534cf05e6b5a20db6aa55198aad842/src/component/spine-component-system.js#L1-L5

No one will go back to ES5, so someone will eventually rewrite the playcanvas-spine project to ES6, I suppose.

This is a cap, or upper bound. Everything below is just fine.

Tools should be fine with it too if we give them consistent configs.

Yea, we can even transpile everything down to ES3. But why? ES6 is setting the standard, and not ES5. And in ES6 you are not allowed to extend a module and a bunch of other things.

I'm going one small step at a time. You are right. There's history and standards I do not know and tools / configs have conflicting settings.

What is the minimum expected node version for engine developers (not consumers or users) and why 18.x ?

Developers in this case are people and systems who will actively build / preview builds of the engine, not consumers of the final build products.

The idea is to remove inconsistencies between tool behaviour running on unsupported versions of node. Most tooling requires at least 16.x. In a few weeks, even 16.x wont be supported. CI system was updated to 18.x and a PR is on the way so tests can be run over a matrix of node versions when required.

This setting is fed back into the package.json so developers get a non-blocking warning to update to a stable version. Some tooling respects this and many people have switchers that swap versions for them between projects.

Next step is to set a ecmascript minimum and enable eslint rules (warning only) for things not allowed beyond es5. This is well trotted territory with the edge cases sorted. The tools will print out a list of the technical debt.

Then pay it once, flip rules to error, code stays clean.

Along the way, we can clean up some configs and duplication.

It would also be helpful to have a clear definitions on what the guarantee of es5 code is. There might be confusion among users and developers alike from what I have read.

Here's a straw man to beat up:

es5 code should only be expected in the final build products processed by the tooling or where code is expected to be consumed directly / processed by the end user (see scripts directory?)

I'll be honest, I'd go much further. The last reasonable use case for having a webgl library supporting es5 disappeared a few years back.

The last reasonable use case for having a webgl library supporting es5 disappeared a few years back.

That is true in general, but unfortunately our scripts in the Editor still depend on ES5 engine, so we need it, till we finalize that upgrade to module world as well.

I have only seen pictures of the editor. That sounds exciting. I hope to learn enough about the library to contribute to that.

In the meantime, if we can get consensus and clear answers to these questions, I'll submit PR's to bring things into alignment.

have only seen pictures of the editor.

create a free account and have a go. Or contribute to the engine instead :)

our official npm version at the moment is 16.15.0, which we used across repos. Ideally we'd aim higher though as we're looking at moving to 18 or so

Also, maybe I can kill 2 stones with one bird by addressing the issues in playcanvas-spine.

  • replace the build system
  • ensure it builds all the es variants

Quick update on playcanvas-spine. Here's what's been causing you issues.

  • example files are bound to the version of the tool / library that created them
  • playcanvas-spine project does not build to any standard, it literally glues 5 IIFE files together.
  • API changes between 3.8 - 4.1 (passing slot.bone instead of slot)

This is not fixed, the issue is that you cannot extend pc namespace as in e.g. here:

pc.extend(pc, function () {
    var SpineComponentSystem = function SpineComponentSystem(app) {
        pc.ComponentSystem.call(this, app);

https://github.com/playcanvas/playcanvas-spine/blob/bb181dd779bda5d38ace58bcc141f51544f92e1c/src/spine.js#L5

Now it "looks" like ES6, but still using the ES5 pattern which is not allowed any longer.

My understanding is that "extend" refers to the method defined in "src/core/core.js".

function extend(target, ex) {

It simply copies enumerable properties recursively. Nothing language specific there.

function extend(target, ex) {
    for (const prop in ex) {
        const copy = ex[prop];

        if (type(copy) === 'object') {
            target[prop] = extend({}, copy);
        } else if (type(copy) === 'array') {
            target[prop] = extend([], copy);
        } else {
            target[prop] = copy;
        }
    }

    return target;
}

You'll see it's exposed here:

export { apps, common, config, data, extend, revision, type, version } from './core/core.js';

The plugin is talking to the instance of the engine running in the browser called "pc". It a global variable so it might more accurately be written as "window.pc".

I'm going to test it out in examples now to give you peace of mind and address any issues.

You do bring up a good point.

There are edge cases to writing a deep "reference only" copy that way. Some maintainers use object.hasownproperty and those bits to make it safe. It's the same reason object based dictionaries are slowly being replaced with maps by Martin (also the performance is 2x).

m8 you overthink it:

cd playcanvas-engine
node
pc = await import("./src/index.js");
> pc.nope = "see"
'see'
> pc.nope 
undefined

image

Spine lib is extending an object, which cannot be extended - nothing about deep references or something.

Analog in browser:

image

Couple of bits.

  • examples havent been updated with new spine build
  • examples builds typescript to es5, rollup does its best but wants es6 input and does conversion back then a conversion down again.
  • command for switching engine versions with temp environment var only works on Mac
  • its easy to add an extra space to env var by mistake making es5 error during build

I am updating this comment with the results of tests for spineboy.

Examples Spine PlayCanvas Engine Status
es5 - umd es5 - iife es5 - playcanvas.dbg.js good
es5 - umd es5 - iife es5 - playcanvas.js good
es5 - umd es5 - iife es5 - playcanvas.prf.js good
es5 - umd es5 - iife es5 - playcanvas.min.js good
es5 - umd es5 - iife es6 - playcanvas.mjs issue
es5 - umd es5 - iife es6 - playcanvas.prf.mjs issue
es5 - umd es5 - iife es6 - playcanvas.min.mjs issue
es5 - umd es5 - iife es6 - playcanvas.dbg.mjs issue

Testing complete

Notes:

No updates to spine plugin in examples has been made since this issue was raised.
A console log message was set at the start of each engine module to ensure they are clearly identified
Some build issues have been corrected 3 weeks ago
"import" can be used on esm, iife, commonjs : not umd modules
umd isnt a module but a form of iife which accomplishes the same thing

Observations:

2 instances of playcanvas engine are created. The first is playcanvas.js, the second one is whatever is being used by the example. it appears that a new instance is created for each iframe (example). Must be to ensure the test is clean.

playcanvas.mjs - issue

(function () {
    if (pc.Application.registerPlugin) {
        var register = function (app) {
            new pc.SpineComponentSystem(app);
        };
        pc.Application.registerPlugin("spine", register);
    } else {
        var app = pc.Application.getApplication();
        var system = new pc.SpineComponentSystem(app);  // <-- point of failure
        app.systems.add(system);
    }
}());

Cause:

spine plugin relies on the window global variable "pc" and the ability to assign properties to it as if it were a plain object. its a module so it is frozen.

See comment above: #5399 (comment)
See commment below: #5399 (comment)

examples "development" commands - issue

es5 and es6 extras build libraries can get mixed when developing with examples.

The some commands used for development do not rebuild enough / clear the environment. Developers are given the last version of playcanvas.extras.(m)js that was used / referenced since the last full build.

This may or may not match the module format of the current ENGINE_PATH environment variable. For instance, full build with ENGINE_PATH=playcanvas.mjs (es6) but then running watch:debug with ENGINE_PATH=playcanvas.js (es5).

The examples load with mixed modules creating undefined behaviour for developers.

playcanvas.js
playcanvas.extras.mjs

To reproduce:

  • tag each playcanvas build with a console log statement for clarity
  • set the ENGINE_PATH environment variable to use ES6 builds
  • in the examples directory, run a full build with "npm run build"
  • then run npm run watch:debug which will use ES5

dev tooling - issue

The webserver locks files under the directory it is serving for security and to map them to memory for speed. Commands and tools that "watch" for changes attempt to update files and are blocked with a permissions error. See develop command.

I am not sure if they continue to function but I suspect that this is the cause of the zombie processes. To complete tests or builds, I am forced to stop the web server, build, then restart the webserver.

It's rather simple, if you have:

<script type="module">
  // whereever that is in your importmap, basically
  // just /playcanvas-engine/src/index.js without build process
  import * as pc from 'playcanvas';
  window.pc = pc;
  ...

it will fail to do pc.extend(pc, ... which the spine lib does, because pc is a frozen Module. Every transformed ES5 build artifact has no issue with the spine lib, because pc is just a good old Object and is not frozen.

You're right and I understand what you wrote the first time. I'm just running through this exhaustively so when it's done, its done for good.

I'm at the half way point but I can give you some solid answers.

  • This spine issue belongs in the spine project and should remain closed.
  • the examples project is one of the coolest projects i've read, state of the art
  • it's build configuration could use some more attention

The current implementation of spine needs to store data and make features available to every app instance that the global "pc" instance might create. It needs to persist it even when those app instances, scenes, or root nodes are destroyed. At the time, a clever and graceful solution was to tuck things neatly into the "pc" variable to make sure its available no matter what happens. If it took a different approach to meet those goals, this issue goes away.

I heard someone is working to modernise this project already.

For the Engine Project, something like the following might help.

If the "pc" variable was a javascript proxy for the real "pc" object we all know, it could forward method calls to a list of registered plugins if it does not exist on the real object. All code stays the same. The proxy exposes named singleton instance storage that will live as long as the real "pc" object and this proxy.

Lets call it a map. Plugins just add themselves to this map. Proxy looks at plugins if a name does not exist on main object. I don't like this design for this problem. I would prefer the plugins proxy themselves so engine does not have the performance overhead.

I used something like this to have a micro game engine (implemented as a proxy) accept and buffer calls from the application (register modules, add textures, etc) while the real game engine was still downloading.

Found a solution for wrapping scripts and plugins in a es6 proxy so they get es5 behaviour when they access globals. I'll try to implement on spline in its current form as a proof of concept. Don't know if this can solve the problem of state and scope bound to one instance of "pc". I like it because the plugin / script has the tiny overhead, not normal calls to the engine.

Since the problem is completely on the Spine side, I will continue to work on it under the issue raised on that repo. See link above.

All sorted. Waiting for the PR review.

If you want to test it in examples, make sure to copy the new spine 3.8 into the scripts/spine in the root of this repo before you build and set engine version in examples. The build will copy that one into the examples folder.

Same file works es5 and es6, in both editor and examples.
Editor demo here: https://playcanvas.com/project/1124022/overview/spine-41-demo