Skip to content

Create scripts added to the registry later at their declared index - #9202

Merged
mvaligursky merged 1 commit into
mainfrom
mv-script-awaiting-order
Aug 21, 2026
Merged

Create scripts added to the registry later at their declared index#9202
mvaligursky merged 1 commit into
mainfrom
mv-script-awaiting-order

Conversation

@mvaligursky

@mvaligursky mvaligursky commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Description

When a script is declared before its script type is registered, the awaiting entry stores ind — the number of script instances the component had at that moment:

this._scriptsIndex[scriptName] = {
    awaiting: true,
    ind: this._scripts.length
};

The deferred sweep in ScriptRegistry#add then creates the instance at that index. By then other awaiting scripts may already have been created and shifted the array, and consecutive awaiting scripts all captured the same index. So the declared order is not preserved. Reproduces with no cloning involved — declaring a, b, c, d, e, f with b, c, e awaiting, then registering their types:

declared: a, b, c, d, e, f
actual  : a, c, e, b, d, f

Fix

Derive the index when the script is actually created, from the _declarationOrder record #9200 added and rebuilds a clone from: the script belongs directly after the script it was declared after, skipping the ones that do not exist yet. That gives the declared order for any registration order, and — unlike a captured index — follows the preceding script if it has since been move()d.

Sharing one rule with the clone reconstruction is what keeps a source and its clone converging on the same order: with two different mechanisms, cloning a component that had been reordered and still had a script pending gave the source [B, X, A] and the clone [B, A, X].

The awaiting entry keeps ind. It is no longer read by the engine, but it is part of the shape of _scriptsIndex entries.

Last of the awaiting-script fixes after #9200 and #9201.

Testing

Three tests in test/framework/components/script/component.test.mjs, all three fail on main:

  • three awaiting scripts, types registered out of their declared order, end up in declared order
  • an awaiting script follows a preceding script that was moved before its type was registered
  • a clone ends up with the same script order as its source, for that same move-then-register case

Checklist

  • I have read the contributing guidelines
  • My code follows the project's coding standards
  • This PR focuses on a single change

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown

Build size report

This PR changes the size of the minified bundles.

Bundle Minified Gzip Brotli
playcanvas.min.js 2372.2 KB (+0.2 KB, +0.01%) 609.6 KB (+0.1 KB, +0.01%) 473.5 KB (−0.1 KB, −0.01%)
playcanvas.min.mjs 2369.6 KB (+0.2 KB, +0.01%) 608.5 KB (+0.1 KB, +0.01%) 472.9 KB (+0.2 KB, +0.04%)

@mvaligursky mvaligursky left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated PR review by Codex (GPT-5).

I found one ordering-state issue and one documentation-placement issue. The predecessor-based insertion works for the covered ordinary names and registration/move cases, but _scriptsIndex is not a sufficient durable declaration-order record for every supported name or after cloning the moved state through #9200.

The submitted focused suite passed (90 tests), ESLint and git diff --check passed, and all current GitHub checks are green. Two targeted probes failed: integer-like names were reordered, and a source/clone diverged when #9200 was combined with this PR after moving the predecessor.

*/
_awaitingInsertIndex(scriptName) {
let previous = null;
for (const name in this._scriptsIndex) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Track declaration order explicitly instead of deriving it from this object. JavaScript enumerates integer-like keys before other string keys, and such script names are currently accepted: declaring ['scriptA', '10', '2', 'scriptB'] on this head ultimately produced ['2', '10', 'scriptA', 'scriptB']. There is also a cross-PR failure with #9200: declare A, B, X(awaiting), move B to index 0, clone, then register X; this helper gives the source [B, X, A], while the clone reconstructed from the stale ind becomes [B, A, X]. Please retain/clone a stable declaration sequence (or otherwise make both reconstruction and deferred insertion share one source of truth).

* @param {number} scriptsLength - The length of the scripts array.
* @private
*/
/**

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P3] Move the new helper and its JSDoc above the existing _insertScriptInstance JSDoc. As written, two documentation blocks are consecutive, so the original Inserts script instance... block is orphaned and _insertScriptInstance no longer has the parameter documentation that immediately preceded it.

@mvaligursky
mvaligursky force-pushed the mv-script-awaiting-order branch from 1ea157f to c591e2a Compare August 20, 2026 13:07
@mvaligursky
mvaligursky changed the base branch from main to mv-script-clone-awaiting August 20, 2026 13:07
@vercel
vercel Bot requested a deployment to Preview – engine-api-docs August 20, 2026 13:08 Abandoned
@vercel
vercel Bot requested a deployment to Preview – engine-api-docs August 20, 2026 13:08 Abandoned
@mvaligursky

Copy link
Copy Markdown
Contributor Author

Both fixed, and the PR is now stacked on #9200 so the two share one ordering rule.

P1 — you were right that _scriptsIndex key order is not a durable declaration-order record, and the cross-PR divergence is the more serious half of it. Rather than patch each side, #9200 now records _declarationOrder explicitly (maintained in create()/destroy(), and reserved in reservedScriptNames so a script cannot shadow it), and both the clone reconstruction and this deferred insertion derive from it. That covers integer-like names, and the move-then-clone-then-register case now converges: source and clone both end up [B, X, A]. Test added for exactly that.

P3 — the helper and its JSDoc now sit above the _insertScriptInstance block instead of between it and its function.

@mvaligursky mvaligursky left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated re-review by Codex (GPT-5).

Re-reviewed the updated commit stacked on #9200. No actionable issues found.

Both previous findings are resolved: insertion now derives from the explicit stable declaration sequence supplied by #9200, covering integer-like names and move/clone/register parity, and the helper JSDoc no longer orphans _insertScriptInstance documentation. The focused stacked ScriptComponent suite passed (96 tests); changed-file ESLint, diff validation, and all current CI checks are green.

When a script is declared before its script type is registered, the awaiting
entry stores `ind`, the number of script instances the component had at that
moment. The deferred sweep in ScriptRegistry#add then creates the instance at
that index, but by then other awaiting scripts may have been created and
shifted the array, and consecutive awaiting scripts all captured the same
index. Registering the types of `a, b, c, d, e, f`, where `b`, `c` and `e` were
awaiting, produced `a, c, e, b, d, f`.

Derive the index when the script is actually created instead, from the same
declaration order the clone is rebuilt from: the script belongs directly after
the script it was declared after, skipping the ones that do not exist yet. That
gives the declared order for any registration order, and follows the preceding
script if it has since been moved.

The awaiting entry keeps `ind`, it is no longer read by the engine but is part
of the shape of `_scriptsIndex` entries.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mvaligursky
mvaligursky merged commit 946db41 into main Aug 21, 2026
10 checks passed
@mvaligursky
mvaligursky deleted the mv-script-awaiting-order branch August 21, 2026 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant