Skip to content

Copy the ESM script attribute data when cloning a script component - #9203

Merged
mvaligursky merged 1 commit into
mainfrom
mv-script-clone-attributes-alias
Aug 21, 2026
Merged

Copy the ESM script attribute data when cloning a script component#9203
mvaligursky merged 1 commit into
mainfrom
mv-script-clone-attributes-alias

Conversation

@mvaligursky

Copy link
Copy Markdown
Contributor

Description

Hardening, no user-visible bug today.

cloneComponent passed the source component's own _attributeDataMap entry straight into the clone's component data:

const attributes = entity.script._attributeDataMap?.get(scriptName) || { };
for (const key in scriptInstance.__attributes) {
    attributes[key] = scriptInstance.__attributes[key];   // writes into the source's object
}

initializeComponentData then does component._scriptsData = data.scripts, so the clone's _scriptsData ends up holding the very object the source reads its attribute values back from — anything writing through one writes through the other.

Nothing mutates it as things stand: __attributes only exists on ScriptType instances, and those never get an _attributeDataMap entry (create only populates the map for non-ScriptType scripts), so the copy loop is a no-op for exactly the scripts the map holds data for. The two happen to be disjoint, which is what keeps the loop above from writing into the source component. That is a fragile thing to rely on, so copy.

Testing

One test in test/framework/components/script/component.test.mjs, fails on main: the clone's attribute data is a distinct object, and writing to it does not change the source's.

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.0 KB, +0.00%) 609.6 KB — 473.4 KB (−0.0 KB, −0.01%)
playcanvas.min.mjs 2369.6 KB (+0.0 KB, +0.00%) 608.5 KB (−0.0 KB, −0.00%) 472.8 KB (−0.1 KB, −0.02%)

@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).

No actionable issues found.

The shallow object copy is at the correct ownership boundary: it prevents the clone's component data from aliasing the source _attributeDataMap entry while preserving the existing value semantics for attribute payloads, which are subsequently interpreted or cloned according to their schemas. The regression test verifies both identity separation and mutation isolation.

Local verification on the exact head: the full focused ScriptComponent suite passed (89 tests), ESLint passed for both changed files, and git diff --check passed. All current GitHub checks are green.

`cloneComponent` passed the source component's own `_attributeDataMap` entry
straight into the clone's component data, so the clone's `_scriptsData` ends
up holding the object the source reads its attribute values back from, and
anything writing through one is writing through the other.

Nothing mutates it today - `__attributes` only exists on ScriptType instances,
which never get an `_attributeDataMap` entry, so the copy loop right below is
a no-op for exactly the scripts the map holds data for - but the sharing is
not intentional and the loop is one step away from writing into the source.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@mvaligursky
mvaligursky force-pushed the mv-script-clone-attributes-alias branch from 2378cba to 488aa31 Compare August 21, 2026 10:09
@mvaligursky
mvaligursky merged commit d0620c4 into main Aug 21, 2026
10 checks passed
@mvaligursky
mvaligursky deleted the mv-script-clone-attributes-alias branch August 21, 2026 10:12
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