Fix ScriptComponent clone dropping scripts awaiting their script type - #9200
Conversation
Build size reportThis PR changes the size of the minified bundles.
|
mvaligursky
left a comment
There was a problem hiding this comment.
Automated PR review by Codex (GPT-5).
No actionable issues found in this PR in isolation.
The fix correctly dereferences each _scriptsIndex entry, reconstructs the missing scripts data required by component initialization, copies awaiting attribute data, and offsets equal captured indices so consecutive awaiting declarations are not reversed during cloning. The clone then retains an awaiting entry and participates in the later registry sweep.
Local verification on the exact head: the full focused ScriptComponent suite passed (91 tests), ESLint passed for both changed files, and git diff --check passed. All current GitHub checks are green.
`cloneComponent` checked `key.awaiting` while iterating `_scriptsIndex` with `for...in`, where `key` is the script name string, so the check was always false and the block was dead code. Cloning an entity therefore dropped any script whose script type was not yet in the registry: when the type was later registered, the deferred registry sweep created the script on the original entity but the clone had no awaiting entry, so it never got it. Restoring the name into `order` is not enough on its own, the awaiting script also needs an entry in `scripts` - `initializeComponentData` looks every name coming from `order` up in there, so the clone would throw a TypeError instead of silently dropping the script. Place the awaiting script directly after the script it was declared after, which is where the deferred creation will put it on the source entity. That needs the declaration order of all scripts, created or not, which neither `_scripts` (reordered by move()) nor the key order of `_scriptsIndex` (integer-like script names are enumerated first) is, so record it explicitly as `_declarationOrder`. Fixes #2796 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cc49f49 to
105dee2
Compare
mvaligursky
left a comment
There was a problem hiding this comment.
Automated re-review by Codex (GPT-5).
Re-reviewed the changes since commit cc49f49. No actionable issues found.
The explicit _declarationOrder record removes the integer-like-key enumeration problem and gives clone reconstruction the same predecessor rule used by the stacked ordering fix. It is maintained across create/destroy, protected as a reserved script name, and reconstructs moved-plus-awaiting clones consistently. The expanded focused ScriptComponent suite passed (93 tests); changed-file ESLint, diff validation, and all current CI checks are green.
Description
ScriptComponentSystem#cloneComponentiterated_scriptsIndexwithfor...inand checkedkey.awaiting, wherekeyis the script name string — so the check was alwaysundefinedand the whole block was dead code:Cloning an entity therefore silently dropped any script whose script type was not in the registry yet. When the type was later registered, the deferred sweep in
ScriptRegistry#addcreated the script on the original entity, but the clone had no awaiting entry so it never got one:Fix
Dereferencing the index entry is not enough on its own:
The awaiting script also needs an entry in
scripts.initializeComponentDatalooks every name fromorderup inscripts(data.scripts[data.order[i]].enabled), so a name inorderbut missing fromscriptsthrowsTypeError: Cannot read properties of undefined (reading 'enabled')— a hard throw out ofentity.clone()in place of the silent drop. The data comes from_scriptsData, which is also where the registry sweep reads the attributes from when it eventually creates the script.The awaiting script needs a well-defined slot. It is placed directly after the script it was declared after, which is where the deferred creation puts it on the source entity (see Create scripts added to the registry later at their declared index #9202), so source and clone converge on the same order.
That needs the declaration order of all scripts, created or not. Neither existing record is that:
_scriptsonly holds created scripts and is reordered bymove(), and the key order of_scriptsIndexis not declaration order for every accepted script name (integer-like keys are enumerated first, so['scriptA', '10', '2', 'scriptB']enumerates as2, 10, scriptA, scriptB). So it is recorded explicitly as_declarationOrder, maintained increate()/destroy().Supersedes #8264, which is the one-line dereference and hits the
TypeErrorabove.Fixes #2796
Testing
Five tests in
test/framework/components/script/component.test.mjs, all five fail onmain:ind, and its declaredenabled/ attribute dataChecklist
🤖 Generated with Claude Code