-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Adds Script scriptName field #7593
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
Conversation
Added warnings for ESM Script classes lacking a static "scriptName" property. Updated the ScriptComponent and ScriptHandler to infer script names and log appropriate debug messages. Enhanced getScriptName function to return the scriptName if defined. Added tests to verify the new behavior.
Updated the ScriptComponent to ensure that a static "scriptName" property is required only for non-ScriptType instances. Modified tests to check for script existence using the `has` method and added a new test to confirm that no warnings are logged when a ScriptType is used. This improves the robustness of script registration and validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds explicit support for naming ESM Script classes via a static "scriptName" field, with appropriate warnings for missing definitions.
- Updated script name resolution to prefer the static "scriptName" property.
- Added warning messages when ESM Scripts do not specify a "scriptName".
- Extended unit tests to validate correct script naming behavior and fallback mechanisms.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/framework/components/script/component.test.mjs | Added tests for warning messages and for correct script name registration. |
| src/framework/script/script.js | Updated getScriptName to return a static "scriptName" if available. |
| src/framework/handlers/script.js | Added warning and fallback logic based on the "scriptName" property of script classes. |
| src/framework/components/script/component.js | Modified script name resolution and warning logic to enforce explicit "scriptName" usage. |
Comments suppressed due to low confidence (1)
src/framework/components/script/component.js:712
- [nitpick] Consider caching the result of getScriptName(scriptType) in a variable to avoid calling it twice in this block, which would improve code clarity.
scriptType.__name ??= toLowerCamelCase(getScriptName(scriptType));
mvaligursky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. It'd be good to update engine supplied scripts https://github.com/playcanvas/engine/tree/main/scripts/esm to this, and also at least one (rotator) in the examples
Separate PR is fine of course.
ESM Scripts now require a `scriptName` field. See playcanvas/engine#7593 This updates the boiler plate to include this new field.
ESM Scripts now require a `scriptName` field. See playcanvas/engine#7593 This updates the boiler plate to include this new field.
Adds support for explicit naming of ESM Scripts using a static
scriptNamefield.This addresses #7580 where class names can get mangled whilst adding support for more verbose script names
and custom casing, both of which have been requested by users.
It handles both cases of loading Scripts as Assets, and also direct insantiation via
entity.script.create(Class)Fixes #7580. Tests included
I confirm I have read the contributing guidelines and signed the Contributor License Agreement.