Fix ESM script registration by name and subclass name collision by mvaligursky · Pull Request #8831 · playcanvas/engine

mvaligursky · 2026-06-03T13:28:32Z

Summary

Fixes two long-standing script name-resolution bugs that share one root cause: names were read through the prototype chain instead of from the class itself.

#8824 — ESM script added to the registry not keyed by its name

ESM scripts declare their name with a static scriptName field. Because it's a class field, it shadows the inherited scriptName accessor ([[Define]] semantics), so the setter that assigns __name never runs. ScriptRegistry.add() read the unset __name and registered the script under a null key — so it couldn't be created by name:

class Test extends Script { static scriptName = 'test'; }
app.scripts.add(Test);
entity.script.create('test'); // silently failed

This broke template instantiation, procedural creation, and async/dynamic script loading for engine-only projects.

#2881 — subclass of a script replaces the superclass

A subclass inherited its base class's already-assigned __name/scriptName, so registering the subclass overwrote the base in the registry (and could trigger an unintended hot-swap):

registerScript(Base);     // registered as 'Base'
registerScript(Derived);  // also resolved to 'Base' -> overwrote Base

Fix

Name resolution now considers only own properties of the class. The logic is centralized in a new internal getScriptRegistryName() helper — own __name → own scriptName → lowerCamelCase class name — and used consistently by registerScript, ScriptRegistry.add, ScriptComponent.create, and the ESM script handler.

Compatibility

No public API signatures change. registerScript, createScript, ScriptRegistry/.add(), Script are unchanged; the new helpers are internal (not exported from index.js).
Conforming scripts (with a declared scriptName, or a name passed to createScript/registerScript) are unaffected.
Only observable change in the undocumented nameless path: registerScript/add now resolve to the lowerCamelCase class name (matching the asset loader and create, which already did) instead of PascalCase.

Tests

Adds test/framework/script/script-registry.test.mjs covering both fixes (ESM add by name, subclass non-collision, explicit-name precedence, nameless camelCase unification). Full suite passes.

Script name resolution previously read `__name`/`scriptName` through the prototype chain, which caused two bugs: - ESM scripts declare their name via a static `scriptName` field. Being a class field, it shadows the inherited `scriptName` accessor with [[Define]] semantics, so the setter that assigns `__name` never runs. `ScriptRegistry.add` read the unset `__name` and registered the script under a null key, so it could not be created by name (e.g. `entity.script.create('test')`), breaking templates/procedural/async workflows. - A subclass inherited its base class's already-assigned `__name`/`scriptName`, so registering the subclass overwrote the base in the registry (and could trigger an unintended hot-swap). Name resolution now considers only own properties of the class. Logic is centralized in `getScriptRegistryName` (own `__name` -> own `scriptName` -> lowerCamelCase class name) and used by `registerScript`, `ScriptRegistry.add`, `ScriptComponent.create`, and the ESM script handler, so all paths resolve the same name. Nameless scripts now consistently use the lowerCamelCase class name (previously `registerScript`/`add` used PascalCase while the asset loader and `create` already used camelCase). Adds dedicated ScriptRegistry tests covering both fixes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

This PR fixes script name-resolution issues in the scripting framework by ensuring script names are resolved from own (non-inherited) class properties, preventing ESM scriptName shadowing bugs and subclass name-collision overwrites in the script registry.

Changes:

Introduces getScriptRegistryName() (own __name → own scriptName → lowerCamelCase class name) and applies it across script registration and creation paths.
Updates ScriptRegistry.add, registerScript, ScriptComponent.create, and the ESM ScriptHandler to use consistent, own-property-only name resolution.
Adds a new test suite covering ESM registration, subclass non-collision, and fallback naming behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
`test/framework/script/script-registry.test.mjs`	Adds coverage for ESM `scriptName` registration, subclass behavior, and consistent fallback naming.
`src/framework/script/script.js`	Adds `toLowerCamelCase` export and `getScriptRegistryName`; tightens `getScriptName` to own `scriptName` only.
`src/framework/script/script-registry.js`	Uses centralized registry-name resolution in `add()` and handles missing-name failure.
`src/framework/script/script-create.js`	Switches `registerScript` name resolution to `getScriptRegistryName`.
`src/framework/handlers/script.js`	Updates ESM script loading to warn based on own `scriptName` and register via resolved registry name.
`src/framework/components/script/component.js`	Aligns ScriptComponent.create() naming/warnings with own-property-only resolution.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+
+        script.__name = scriptName;

        if (this._scripts.hasOwnProperty(scriptName)) {


Address review feedback on edge cases where a script name cannot be resolved (e.g. an anonymous class with an empty `.name` and no static `scriptName`): - `toLowerCamelCase` returns the input unchanged for empty/falsy values instead of throwing on `str[0]`. - `registerScript` logs an error and returns early instead of registering under an undefined name and pushing to `ScriptTypes`. - `ScriptComponent.create` returns null (and logs) rather than indexing the instance under an `undefined`/empty key. - the ESM script handler skips exports whose name cannot be resolved. Adds tests for the registerScript and ScriptComponent.create fail-fast paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

mvaligursky · 2026-06-03T13:53:00Z

Thanks for the review @copilot. Addressed the name-resolution edge cases (anonymous class with an empty .name and no static scriptName) in 05318dc:

Addressed (threads resolved):

toLowerCamelCase now returns the input unchanged for empty/falsy values instead of throwing on str[0]. This also defuses the two warning-string crashes (component.js and handlers/script.js).
registerScript logs an error and returns early when no name resolves, so it no longer assigns __name = undefined, calls add(), or pushes a nameless entry to ScriptTypes.
ScriptComponent.create returns null (and logs) instead of indexing the instance under an undefined/empty key (this.undefined).
ESM script handler skips exports whose name cannot be resolved rather than registering under an invalid key.

Added tests for the registerScript and ScriptComponent.create fail-fast paths.

Not addressed (left open):

The this._scripts.hasOwnProperty(...) suggestion in script-registry.js. It's a valid hardening point, but that line is pre-existing (untouched by this PR) and unrelated to the name-resolution fix, so it's out of scope here. Happy to do it as a separate small PR.

Copilot · 2026-06-03T13:53:14Z

@mvaligursky I've opened a new pull request, #8832, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

+ * @param {string} str - The string to convert.
+ * @returns {string} The converted string.
+ */
+export const toLowerCamelCase = str => (str ? str[0].toLowerCase() + str.substring(1) : str);


+            // a `scriptName` declared on THIS class; an inherited one would belong to a base class
+            const ownScriptName = Object.prototype.hasOwnProperty.call(scriptType, 'scriptName') && scriptType.scriptName;
+
+            if (!(scriptType.prototype instanceof ScriptType) && !ownScriptName) {
+                Debug.warnOnce(`The Script class "${inferredScriptName}" must have a static "scriptName" property: \`${inferredScriptName}.scriptName = "${toLowerCamelCase(inferredScriptName)}";\`. This will be an error in future versions of PlayCanvas.`);
            }


+                    // a `scriptName` declared on THIS class; an inherited one belongs to a base
+                    const ownScriptName = Object.prototype.hasOwnProperty.call(scriptClass, 'scriptName') && scriptClass.scriptName;

-                    if (!scriptClass.scriptName) {
-                        Debug.warnOnce(`The Script class "${scriptClass.name}" must have a static "scriptName" property: \`${scriptClass.name}.scriptName = "${lowerCamelCaseName}";\`. This will be an error in future versions of PlayCanvas.`);
+                    if (!ownScriptName) {
+                        Debug.warnOnce(`The Script class "${scriptClass.name}" must have a static "scriptName" property: \`${scriptClass.name}.scriptName = "${toLowerCamelCase(scriptClass.name)}";\`. This will be an error in future versions of PlayCanvas.`);


Maksims · 2026-06-03T14:04:46Z

app.scripts.add(Script) - now works as intended.
Also tested on large project if any regressions, no regressions found.
Thank you!

mvaligursky self-assigned this Jun 3, 2026

vercel Bot deployed to Preview – engine-api-docs June 3, 2026 13:28 View deployment

mvaligursky mentioned this pull request Jun 3, 2026

Document engine-only ESM script registration playcanvas/developer-site#1007

Open

vercel Bot deployed to Preview – engine June 3, 2026 13:29 View deployment

mvaligursky requested review from Maksims and Copilot June 3, 2026 13:29

Copilot started reviewing on behalf of mvaligursky June 3, 2026 13:30 View session

mvaligursky added the area: scripts label Jun 3, 2026

mvaligursky requested a review from kpal81xd June 3, 2026 13:36

Copilot AI reviewed Jun 3, 2026

View reviewed changes

vercel Bot deployed to Preview – engine-api-docs June 3, 2026 13:53 View deployment

Copilot AI mentioned this pull request Jun 3, 2026

[WIP] Fix ESM script registration by name and subclass name collision #8832

Closed

vercel Bot deployed to Preview – engine June 3, 2026 13:53 View deployment

mvaligursky requested a review from Copilot June 3, 2026 13:54

Copilot started reviewing on behalf of mvaligursky June 3, 2026 13:54 View session

Copilot AI reviewed Jun 3, 2026

View reviewed changes

Maksims approved these changes Jun 3, 2026

View reviewed changes

mvaligursky merged commit 1e6bb17 into main Jun 3, 2026
9 checks passed

mvaligursky deleted the mv-esm-script-registry-name branch June 3, 2026 14:31

mvaligursky mentioned this pull request Jun 3, 2026

Use a Map for the script registry and guard reserved names in add() #8835

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ESM script registration by name and subclass name collision#8831

Fix ESM script registration by name and subclass name collision#8831
mvaligursky merged 2 commits into
mainfrom
mv-esm-script-registry-name

mvaligursky commented Jun 3, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mvaligursky commented Jun 3, 2026

Uh oh!

Copilot AI commented Jun 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Maksims commented Jun 3, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		script.__name = scriptName;

		if (this._scripts.hasOwnProperty(scriptName)) {

Conversation

mvaligursky commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

#8824 — ESM script added to the registry not keyed by its name

#2881 — subclass of a script replaces the superclass

Fix

Compatibility

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mvaligursky commented Jun 3, 2026

Uh oh!

Copilot AI commented Jun 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Maksims commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mvaligursky commented Jun 3, 2026 •

edited

Loading

Maksims commented Jun 3, 2026 •

edited

Loading