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?
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()
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.
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:
-
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.
-
Accept that /examples/ is pretty much ES5 and if you try ES6, every simple ES5 script may crash it
-
Rewrite everything into ES6 and tell users to use bundlers/transpilers for backwards-compatibility
-
Have
*.js
and*.mjs
files next to it (functionally identical) - this may also make (1) easier
Another ES6 issue in Spine:
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:
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);
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".
Line 85 in 128b0df
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:
Line 17 in 128b0df
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).
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