ESM script base class #6367
ESM script base class #6367
Conversation
marklundin
requested review from
Maksims,
willeastcott,
slimbuck,
kpal81xd and
mvaligursky
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
What would be the impact to users in terms of migrations or API changes? |
|
| script.off('enable', onFirstEnabled); | ||
| } | ||
| }; | ||
| script.on('enable', onFirstEnabled); |
| script[name] = []; | ||
| for (let i = 0, len = value.length; i < len; i++) { | ||
| script[name].push(rawToValue(app, args, value[i], old ? old[i] : null)); | ||
| } |
There was a problem hiding this comment.
is it possible to construct and populate the array first then assign it to the script attribute?
this could be useful for users that have implemented a setter, for example.
| function getReservedScriptNames() { | ||
| return reservedScriptNames; | ||
| } |
There was a problem hiding this comment.
wondering why not just export reservedScriptNames?
There was a problem hiding this comment.
Speaking of which - the Editor UI should not allow creating a script with a reserved name, e.g. "script".
There was a problem hiding this comment.
This is not part of this PR, it's just a renamed file https://github.com/playcanvas/engine/blob/main/src/framework/script/script.js#L11-L23
There was a problem hiding this comment.
Yeah, I should create a separate issue in Editor repo.
| function getReservedScriptNames() { | ||
| return reservedScriptNames; | ||
| } | ||
| export class Script extends EventHandler { |
There was a problem hiding this comment.
Might be worth adding some documentation for this class explaining how it fits into the broader scripting context?
| ScriptType.prototype.initScriptType.call(this, args); | ||
| }; | ||
|
|
||
| scriptType.prototype = Object.create(ScriptType.prototype); |
There was a problem hiding this comment.
what's the purpose/need of this custom class/initialisation code?
There was a problem hiding this comment.
This is legacy code, and it's not clear why ScriptType are initialized this way. This was before ES classes and super syntax, so it's likely this is trying to polyfill some functionality. I don't really want to change how this works or modify anything related to ScriptType as we will eventually deprecate this
| * }; | ||
| * @category Script | ||
| */ | ||
| function createScript(name, app) { |
There was a problem hiding this comment.
I understand naming is probably a bit tricky given the existing codebase, but I feel like the name createScript is misleading and would more accurately be called createScriptType or similar.
There was a problem hiding this comment.
This is public API var Rotator = pc.createScript('Rotator') and can't be changed without updating everyones projects
New Script Base class
This PR adds a new base class
Scriptto replace the currentScriptTypeclass which is incompatible with ESM class syntax.The
Scriptclass extracts a small subset of features fromScriptTypeand keeps anything related toScriptAttributesinScriptType.ScriptTypenow inherits fromScript. This update minimizes the code changes required in the component system.Files Changes
script.jsrenamed toscript-create.jsscript.jscontainsScriptbase classscript-type.jscontains legacyScriptTypeclassAPI Changes
ScriptAttributes.rawToValueis now exposed as a utility. This will also be used by the launcher pageScriptRegistry.addSchema/getSchema- This stores a map of the the attribute schema<scriptName, schema>Tests
Passes the ScriptType tests in
npm run test:karmaI confirm I have read the contributing guidelines and signed the Contributor License Agreement.