Keep the options an awaiting script was declared with - #9201
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).
I found one remaining correctness gap in the same public awaiting-script path. The per-component local fixes the cross-component attribute leak, and serialized component data now carries enabled correctly, but options supplied through ScriptComponent#create() are still discarded before this sweep.
The submitted focused suite passed (90 tests), ESLint and git diff --check passed, and all current GitHub checks are green. A targeted programmatic-awaiting regression probe failed as described inline.
|
|
||
| // the data the script was declared with, if any - note this must be read per | ||
| // component, a component without it must not inherit the previous one's data | ||
| const scriptData = component._scriptsData?.[scriptName]; |
There was a problem hiding this comment.
[P1] Preserve options from programmatic awaiting creation. _scriptsData is populated by initializeComponentData, but the public component.create('later', { enabled: false, attributes: { speed: 42 } }) path stores only { awaiting, ind } when the type is absent. Consequently scriptData is undefined here, the instance is created enabled, initialize() runs, and its attributes are lost. A focused probe on this head reproduced _enabled === true instead of false. Please persist the original create() options in the awaiting entry (or shared component data) and consume them here as well.
71e78d4 to
d11cb26
Compare
|
Fixed in d11cb26 — good catch, that is the same bug through the public entry point. The awaiting entry now keeps what |
mvaligursky
left a comment
There was a problem hiding this comment.
Automated re-review by Codex (GPT-5).
Re-reviewed the changes since commit 71e78d4. The previous finding is resolved for enabled and attributes: programmatic awaiting entries retain those values, and the registry sweep consumes the per-component fallback correctly. The focused ScriptComponent suite passed (91 tests); changed-file ESLint, diff validation, and all current CI checks are green.
One remaining public-create parity gap was exposed by a targeted probe and is noted inline.
| // ScriptRegistry#add has nothing else to go on for a script that was not declared | ||
| // through component data | ||
| enabled: args.hasOwnProperty('enabled') ? args.enabled : true, | ||
| attributes: args.attributes |
There was a problem hiding this comment.
[P1] Preserve properties for programmatic awaiting creation
ScriptComponent#create publicly documents args.properties and applies it with Object.assign when the type is already registered, but this awaiting record keeps only enabled and attributes, and the later registry call likewise cannot forward properties. A targeted probe using create('loadedLater', { properties: { customValue: 42 } }) produced an instance whose customValue was undefined after registration. Please store and forward this public option as well so delayed creation has the same semantics as eager creation. When reconciling this PR with #9200/#9202, the same data also needs to survive cloning while retaining #9202's computed insertion index.
d11cb26 to
5dcc235
Compare
|
Fixed in 5dcc235 — On the parenthetical about #9200/#9202: the PR is now stacked on #9200 (the whole chain is linear, 9200 → 9201 → 9202, so nothing conflicts).
|
mvaligursky
left a comment
There was a problem hiding this comment.
Automated re-review by Codex (GPT-5).
Re-reviewed the latest commit stacked on #9200. The previous properties finding is resolved, including deferred creation on the original component, and the deliberate omission of assigned properties from clones matches existing ordinary-script clone semantics. The focused stacked ScriptComponent suite passed (97 tests); changed-file ESLint, diff validation, and all current CI checks are green.
One remaining destroy/recreate state bug was exposed by a targeted lifecycle probe and is noted inline.
| // the data the script was declared with - component data if it came from | ||
| // there, otherwise what create() was called with. Note this must be read per | ||
| // component, a component without it must not inherit the previous one's data | ||
| const scriptData = component._scriptsData?.[scriptName] ?? indexData; |
There was a problem hiding this comment.
[P1] Do not let stale component data override recreated options
ScriptComponent#destroy removes the index entry but does not remove _scriptsData[scriptName], so this presence check cannot tell where the current awaiting declaration came from. Reproduce by declaring missing loadedLater in component data with enabled: true, destroying it, then calling create('loadedLater', { enabled: false, attributes: { disableEntity: false } }): after registration this line selects the old component data, and a targeted probe observed the recreated instance enabled and initialized instead of disabled. cloneComponent has the same _scriptsData ?? indexData precedence, so a clone also retains the stale values. Please clear the raw entry when the declaration is destroyed or otherwise track the current declaration's source so both the sweep and clone use the newly supplied options.
5dcc235 to
64fe7e0
Compare
|
Fixed in 64fe7e0, and you were right that this needed the "track the current declaration's source" half rather than a targeted patch — so the I did not clear Nothing is lost from the eager path: Test added for the destroy/recreate case asserting both the entity and a clone of it. |
mvaligursky
left a comment
There was a problem hiding this comment.
Automated re-review by Codex (GPT-5).
No actionable issues found in the latest update.
The previous stale-data finding is resolved at the correct ownership boundary: the current awaiting index entry is now the sole source for deferred creation and clone reconstruction, so destroy/recreate cannot resurrect an earlier declaration and no caller-owned _scriptsData object needs to be mutated. The new regression test covers both the source and clone paths.
Local verification on the exact head: the stacked ScriptComponent suite passed (98 tests), changed-file ESLint passed, and git diff --check passed. The two GitHub checks currently registered for this amended head are both green; the broader Actions checks were not yet listed at review time.
When a script type is registered after the components referencing it have been
created, the deferred sweep in ScriptRegistry#add creates the script instances.
It read the declared attributes out of `_scriptsData` but never the declared
`enabled` flag, so `ScriptComponent#create` fell back to its `true` default: a
script declared `enabled: false` came back enabled and got initialized, unlike
the same script going through the eager `initializeComponentData` path.
`_scriptsData` is only populated for scripts that came from component data, so
a script created through the public `create('later', { enabled: false })` had
its enabled state, attributes and assigned properties dropped as well - the
awaiting entry kept nothing but `awaiting` and `ind`. Keep all three on it, and
consume them both in the sweep and when cloning, so deferred creation ends up
with the same instance as eager creation would have.
The `attributes` local in the sweep was also declared outside the component
loop and only reassigned when the component had `_scriptsData`, so a component
without any declared data inherited the attributes of the previous component in
the loop. Read the data from a per-component local instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
64fe7e0 to
fa1aa39
Compare
Description
When a script type is registered after the components referencing it were created, the deferred sweep in
ScriptRegistry#addcreates the script instances. It read the declared attributes out of_scriptsDatabut never the declaredenabledflag, soScriptComponent#createfell back to itstruedefault. A script declaredenabled: falsecame back enabled and got initialized — unlike the same script going through the eagerinitializeComponentDatapath.Reproduces with no cloning involved:
_scriptsDatais only populated for scripts that came from component data, so a script created through the publiccreate()path fared worse still — the awaiting entry kept nothing but{ awaiting, ind }, soenabled,attributesand the assignedpropertieswere all dropped:Fix
The awaiting entry keeps all three, and is the only thing the deferred creation and
cloneComponentread. So deferred creation ends up with the instance eager creation would have produced, on the entity and on a clone of it.Reading
_scriptsDatawas the underlying problem rather than an incomplete fix:destroy()drops the index entry but leaves_scriptsData[name]behind, so afterdestroy('later')+create('later', { enabled: false })the sweep would resurrect the old declaration's valuesattributeslocal was declared outside the component loop and only reassigned when the component had_scriptsData, so a component with no declared data for that script inherited whichever earlier component in the loop had someBoth fall out of using the awaiting entry, which is created by the declaration that is actually standing, on the component that made it. Nothing needs to clear
_scriptsData, so no caller-suppliedscriptsobject gets mutated.The eager path loses nothing by this:
initializeComponentDatapasses the_scriptsDatavalues intocreate(), so the awaiting entry captures exactly what was declared.Testing
Five tests in
test/framework/components/script/component.test.mjs, all five fail without this commit:enabled: falsestays disabled and is not initialized when its type is registered latercreate()before its type exists keeps itsenabledflag, attributes and propertiesNote
propertiesis deliberately not threaded through cloning. It has no representation in serialized component data, andcloneComponentdoes not carry assigned properties for scripts that already exist either — doing it for awaiting scripts only would make them clone richer than ordinary ones. The clone carriesenabledandattributes, which do have a component data representation.Checklist
🤖 Generated with Claude Code