-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
ES Module Scripts #4767
Comments
Would be great to see an example of most minimal script, as well as a script that uses attributes, and some few other features in the code. |
Hi Will! I'm thrilled to see open discussion about it. Let me share how we do stuff, working around pc quirks. We use webpack bundling and imports inside webstorm IDE, which works very good with jsdoc, so we have full intellisense as if working with typescript (well, almost) We also use inheritance in rare cases, also with attributes, and here is how:
|
we also don't use script hotswap, since it's a chore to write around, since I have to lay off my current state, save it and return to new script, but we do use swap method to live-reload whole app after any change, since we're using file watchers and rest API to load bundle into playcanvas editor. for that, we have reload on swap script:
|
regarding vs code or monaco I don't really know why use them. I would better look at IDE headliner, like intellij, since they are just better IDE's, and they have better intellisense. but that's like my humble opinion. |
Be able to have the script attributes auto complete with type in any code editor. The way they are currently, doesn't allow code editors to infer the property or the type |
There is another issue with the Editor that we would need to consider, there's no real folder structure. Each asset is in a folder with the name as the resource id. This would make importing other scripts/files nearly impossible unless we change this somehow or have a build step for each launch |
@yaustar That's a really good point. Obviously, modules need to specify import paths. So I'm not sure how we'd support that... |
Did the old Scripts V1 workflow and the 'scripts folder' specialised in that respect where they were stored differently? Not entirely sure if that's something we want to go back to. |
For imports, worth looking at import maps. If writing a script would require to import all dependencies instead of current approach, this will have worse learning and usability curve. |
This is really exciting to see. I've spent a bit of time thinking about this so here's a braindump... Ideas for a Module TypeJust as a thought starter, I've always thought Decorators are a good candidate for this kind of thing. Similar to Unity Attributes Consider the following: import { attribute, inspectable } from 'playcanvas'
@inspectable("My Awesome Class", "This description is surfaced to the editor")
class MyClass {
@attribute("This attribute title is exposed to the editor")
nSomeValue= "This is a default value"
initialize(){}
destroy(){}
update(){}
} Of course this is still a Stage 3 Proposal so without TypeScript or other transpiler the same code above would reduce to a slightly more clunky: import { attribute, inspectable } from 'playcanvas'
class MyClass {
nSomeValue= 'This is a default value'
initialize(){}
destroy(){}
update(){}
}
inspectable("My Awesome Class", "This description is surfaced to the editor")(MyClass)
attribute('This title is exposed to the editor')(MyClass.prototype.nSomeValue) The decorators are composable so you can includes things like constraints. The nice thing about this is it's self documenting. Thoughts on ES ModulesModule resolution Problem import moment from 'moment' becomes import moment from 'https://unpkg.com/moment' It doesn't allows you to modify the path resolution algorithm so that a relative path like How to solve this:
Transpilers Potential issues with ES Modules
import moment from 'https://unpkg.com/moment' There are obvious pro's/cons in relying on 3rd party CDN's at run time. Some issues being outages and also allowing non version locked modules which can suddenly be upgraded and subsequently introduce breaking changes for your app. This is not obviously something specific to ES modules per-se but the ability to import modules directly is likely to make the problem more prevalent. This is a persuasive argument for bundling 3rd party dependencies so you always have a static shippable build |
I've just realized that scripts in the launch.playcanvas.com environment are actually hosted with the same paths as the asset registry. So relative import paths would work in this scenario, however downloaded projects and I assume published projects do not have the same paths, so the above issue with the import maps still stands |
You loose the ability to ask for the default value without actually creating an instance? There is also an issue that PC fires events I think ES6 is powerful enough for nice syntax + types already, e.g: import * as pc from 'playcanvas';
import {
ScriptType,
Vec3,
EVENT_KEYDOWN,
KEY_SPACE,
} from 'playcanvas';
export class MouseInput extends ScriptType {
static fromWorldPoint = new Vec3();
static toWorldPoint = new Vec3();
static worldDiff = new Vec3();
/** @type {number} */
orbitSensitivity;
/** @type {number} */
distanceSensitivity;
static {
this.attributes.add('orbitSensitivity', {
type: 'number',
default: 0.3,
title: 'Orbit Sensitivity',
description: 'How fast the camera moves around the orbit. Higher is faster'
});
this.attributes.add('distanceSensitivity', {
type: 'number',
default: 0.15,
title: 'Distance Sensitivity',
description: 'How fast the camera moves in and out. Higher is faster'
});
}
// initialize code called once per entity
initialize() {
// <snip>
}
} The static initialization block is a ES2022 feature, but it can also be simulated like: static _ = (
this.attributes.add('orbitSensitivity', {
type: 'number',
default: 0.3,
title: 'Orbit Sensitivity',
description: 'How fast the camera moves around the orbit. Higher is faster'
}),
this.attributes.add('distanceSensitivity', {
type: 'number',
default: 0.15,
title: 'Distance Sensitivity',
description: 'How fast the camera moves in and out. Higher is faster'
})
); Or of course just putting the attributes under the class declaration, but I think having it "inside" the class helps the code flow a bit. Besides the import maps issue that depends on specific server setups / file distribution styles, isn't the current ES6 class system rather capable? What are the current pain points? |
JavaScript devs do what they always do: polyfill In this case you can use: https://github.com/guybedford/es-module-shims By default it supports CSS and JSON loading via modules, but with a little rewrite, you can even make it load frag/vert e.g. This was a limitation that triggered the change in e.g. this PR: #3850 So right now via import baseNineSliced from './baseNineSliced.frag';
import style from './style.css';
const preFrag = document.createElement('pre');
preFrag.innerText = baseNineSliced;
document.body.append('preFrag', preFrag);
const preStyle = document.createElement('pre');
preStyle.innerText = style;
document.body.append("style", preStyle); To test for yourself, I made a little zip:
The polyfill is doing way more extra work than a programmable resolution hook and yet I barely feel that its shimmed (loading PC, PCUI, pc-observer all as modules + my extra code modules). So their performance argumentation is utter non-sense to me. Well, too bad that misconceptions of a few people drag down the power of the native web environment. At least we can always polyfill anyway, because JS itself is so powerful. just wanted to point out that there is a "point three" in your list |
Nice find on the es shims @kungfooman. Yup this is essentially what I meant by a pre-processing step that re-map or re-write imports. Although that probably wasn't clear 😆
You'd have to gauge the additional overhead incurred by rewriting all these imports at runtime tho, there's probably many cases where this isn't huge, but my guess is it wouldn't scale well to apps with large or complex dependency trees, and it would likely end up degrading the perceived load time. Definitely worth investigating though to measure actual impact, but in my mind any solution that results in an increased time to FCP should be avoided where possible, especially if it could be done ahead-of-time in a publish/build step or even better, avoided altogether. I hadn't actually realized Import Maps weren't supported on FF or Safari, so you're right, in any case this sort of feature would need to be polyfilled at least over the short term. |
Yep you're right, the property decorator would of course require class instantiation. I guess the solution would be something like |
@marklundin Yes, I see your point about First Contentful Paint, my idea is mostly to level Browser technology with bundlers. As soon it works in the browser the way devs are used to by e.g. My current setup loads 711 *.js module files at ~1s and with shim it takes ~1.5s to FCP, which is only for the slow ducks here (Safari / Firefox). Extra time also happens for Chrome, if you use natively unsupported functionality like importing vert/frag files. The unshimmed module loading time also feels too long for me, so for a release build all of it should be just bundeled, tree-shaked, tersered and so on 😅. This is the time I actually want The good thing about ES5 is that you can just concat it all together, so in the context of PlayCanvas Editor there won't be a way around a more involved "module build step" for lowest FCP in any case or to accept the shim extra time until browsers catch up (for Safari I read Import Maps feature is in Technical Preview e.g.). |
I tested hot reloading a bit and it sorta works, just open http://127.0.0.1/playcanvas-test-es/es6.html and execute this in devtools: const url = window.location.origin + '/playcanvas-test-es/es6/rotate.js';
const { Rotate } = await import(url + "?now=" + Date.now());
pc.registerScript(Rotate, Rotate.registerName); "Sorta" because only the I updated this repo that I created some time ago if someone wants to play around with it: https://github.com/kungfooman/playcanvas-test-es (huge thanks to @LeXXik since I started this using his Ammo-Debug-Drawer) |
Thanks for sharing @kungfooman. IIRC dynamic imports can't be statically analysed and are therefore ineligible for tree shaking. This is probably ok when developers explicitly use them, but not suitable at the editor/engine level. My concern with the es import map polyfill is that the path resolution feature isn't part of the spec, so even when all browser support it, you'd still need the polyfill for the non-standard functionality. I'd still be keen to know the perf on a complex tree that makes heavy use of rewriting the import maps |
Forget about dynamic import problems for Hot Code Reloading. The dynamic import is barely code that the PC Editor would trigger e.g. over WebSocket communication after a script is saved: to import the latest version and nothing else. Everything is and remains analyzable statically. About the spec on https://github.com/WICG/import-maps#acknowledgments:
The same guy who developed the So basically if enough people rely on a feature, this is what browsers will eventually support natively (just like jQuery and The spec will probably evolve a lot and then rather in a direction that is of use to most devs by informing browser vendors how JS developers would like/need it. |
Excerpt from #924: It would be great to have TypeScript support with code completion, syntax and error highlighting. Regarding code completion and error highlighting, I would take a look at https://github.com/coder/code-server if you don't know about it - extracting the already-proven VS Code browser is a no-brainer to me. I think you could still maintain complete control over the cloud editor IDE with minimal effort. If you are open to a professional partnership then you should reach out to https://coder.com/ and see if they can help. Thanks, and cheers! |
The VS Code browser is called Monaco and PlayCanvas Editor is already using it. You have everything that TS offers, but none of the downsides. Q: What did the TypeScript developer say when asked about their code? |
I agree with the need for TypeScript. I don't personally use it, but many people do. @ellthompson @yaustar I'd be keen to hear what the proposal is for this. |
The current plan (subject to change) is to support modules by default and with that, support BOTH JS and TS workflows which will involve some sort of build step when opening the launch tab. Again, still VERY early stages. |
Super cool. I think that opens up a lot of potential. It would be great if there was some sort of semantic distinction between source and compiled files, so that source files could be attached to and associated with entities, but compiled scripts are not. One of the issues we had with the Package Manger is that whilst I could compile scripts into a single bundle, they would lose the reference of which specific entity they were attached to |
@yaustar I do need it occasionally, but mostly in projects that were done by another developer, where I don't know where the script is attached to. |
@LeXXik Doesn't the search help with that? Or is it more of a 'flow' when investigating stuff? |
Yes, it does help with that. Was that the question? 😅 I mostly use it during investigations/debugging. |
The question was if on the script asset, find references was needed/still needed if we have the search in hierarchy |
Another question concern that has just popped up: At the moment, attribute parsing of scripts in the Editor is done on a per script asset basis. This has caused issues like playcanvas/editor#760 where constants or base classes are defined in other files. With this system, would the attribute parsing take into account of the imports/exports or would it bundle the scripts first and then parse them? |
ES6 is forcing us to be more careful with scripts aswell. We cannot simply extend The error is triggered when loading the WebXR VR Lab via ES6: https://developer.playcanvas.com/en/tutorials/webxr-vr-lab/ Probably possible to write a regex to warn PC Editor users of this anti-pattern (to make it future proof), or just let everyone figure that out for themselves with crashing/non-working scenes. But what should be the suggested/better/working alternative? Edit: simple regex would do: You simply can't assign anything: This pattern also doesn't work: |
We're facing the same issue with building engine examples using the es6 module version of the engine. @ellthompson has done some exploration, but I don't think we have a solution at the moment. |
Technically you can just re-export the entire module: <body></body>
<script>
function importFile(content) {
return "data:text/javascript;base64," + btoa(content);
}
const imports = {
playcanvasOriginal: "/playcanvas-engine/src/index.js",
playcanvas: importFile(`
export * from "playcanvasOriginal";
export class WhateverPeopleMiss {
xyz = 123;
}
`)
};
const importmap = document.createElement("script");
importmap.type = "importmap";
importmap.textContent = JSON.stringify({imports});
document.body.appendChild(importmap);
</script>
<script type="module">
import * as pcOriginal from "playcanvasOriginal";
import * as pc from "playcanvas";
Object.assign(window, {
pcOriginal,
pc,
});
</script> Test: So even though ES6 modules are frozen, we can basically just extend them anyway (by exporting everything from another module + our own stuff). I had a look at the engine examples and I kinda don't like the way it evolved:
As example, it should simply be And then the entire engine is already a proper MJS module, why do we need a build step at all? It seems overly complicated and confusing to me with all the extra steps. |
Bit of a random bump, but we're also really in favor of the TypeScript decorator approach similar to what @marklundin suggested for declaring attributes and scripts. Here's an example of how we write our scripts: Results in: Declaring attributes this way feels pretty intuitive. It's somewhat reminiscent of Unity's SerializeField. |
The decorator approach is nice, but it's still only a stage 3 proposal and we don't want to force people down a TS route. We do however have something very similar using static JSDoc style tags to declare attributes. It works in a very similar way as your example, doesn't incur any runtime overhead and is compatible with vanilla JS. Feedback is welcome :D |
This is very subjective and taste-dependent. Also, the example you've provided above does not fully represent the UI you've shown, as in UI you have titles on fields, in code you don't. It is best to avoid complex conditioning, magical rules and behaviours as much as possible. |
I mean absolutely, it's our own personal taste. None of it is common sense and nowhere did I suggest that it is. We prefer to use a C# attribute style approach, because it allows us to attach information to fields which seems like a good fit for PlayCanvas script attributes. This is what I meant by it being intuitive: there's tight coupling between a decorator and the field it's attached to. The screenshot is meant to demonstrate how the traditional attribute declarations (i.e. I'm going to preface this by saying that the screenshot is only supposed to demonstrate the use of attribute decorators in general. The additional features that our system supports are not relevant to the discussion here, so I understand the confusion. I do want to answer your questions though, so here it goes. Our system processes attributes in a way that we find helpful. Some of the things it does:
Again, we are not suggesting that PlayCanvas should do things our way. We just like doing things this way. I understand that TypeScript isn't for everyone and as such the decorator approach isn't a "must-have", but I wanted to weigh in that we find it very helpful, so maybe it can help steer the new attribute declaration style for the future. |
The current PlayCanvas script format was introduced in July 2016. PlayCanvas scripts are snippets of JavaScript that are (asynchronously) loaded and then, once loaded, immediately executed in global application scope.
The current PlayCanvas script format has a number of problems:
import
statement.import
call), thepc
namespace doesn't exist. The script would need to import what it needs.ScriptType
-based scripts. For example, attributes don't resolve correctly. An ES Module containing a class might be a better approach.So I propose that we investigate the development of a new script format that:
I welcome comments from the community on this proposal.
The text was updated successfully, but these errors were encountered: