Skip to content

fix(compat): avoid mutating user-provided style objects when appending px#5117

Open
JSap0914 wants to merge 2 commits into
preactjs:mainfrom
JSap0914:fix/clone-element-key-ref
Open

fix(compat): avoid mutating user-provided style objects when appending px#5117
JSap0914 wants to merge 2 commits into
preactjs:mainfrom
JSap0914:fix/clone-element-key-ref

Conversation

@JSap0914

Copy link
Copy Markdown

Bug

handleDomVNode in compat/src/render.js mutates the style object supplied by the caller when appending px to numeric values:

// before
if (i === "style" && typeof value === "object") {
    for (let key in value) {
        if (typeof value[key] === "number" && !IS_NON_DIMENSIONAL.test(key)) {
            value[key] += "px"; // ← mutates props.style in place
        }
    }
}

value is a direct reference to props[i] (the original object passed by the user). After this loop the caller's object is permanently modified—margin: 10 becomes margin: "10px"—which is an unexpected side effect.

Fix

Shallow-copy the style object before the loop so mutations stay local to normalizedProps:

value = assign({}, value);

The px-append behaviour and the rendered output are unchanged; only the caller's object is now left untouched.

Verification

npx vitest run compat/test/browser/render.test.jsx

New test: "should not mutate the original style object when appending px" — this test failed before the fix (style.margin was mutated to "10px") and passes after it.

All 46 existing compat-render tests continue to pass; full suite: 1242 passed, 12 skipped across 101 test files.

AI-assisted contribution.

…g px

handleDomVNode() was directly mutating the style object passed by the
user via value[key] += 'px'. This permanently modifies the caller's
original object—an unexpected side effect.

Fix by shallow-copying the style object before the per-key px loop.
The existing px-append behaviour is preserved; the user's object is
left unchanged.
Copilot AI review requested due to automatic review settings June 17, 2026 12:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

github-actions Bot commented Jun 17, 2026

Copy link
Copy Markdown

📊 Tachometer Benchmark Results

Summary

A summary of the benchmark results will show here once they finish.

Results

The full results of your benchmarks will show here once they finish.

tachometer-reporter-action v2 for CI

Comment thread compat/src/render.js Outdated
}

if (i === 'style' && typeof value === 'object') {
value = assign({}, value);

@marvinhagemeister marvinhagemeister Jun 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The PR description says:

(...) mutates the style object supplied by the caller when appending px to numeric values

which hints that it's only a problem when px is appended. The change here though is broader. It also applies when no px is appended too. That seems wasteful in terms of performance that we always pay the cost of cloning an object for that too.

Comment thread compat/test/browser/render.test.jsx Outdated
Comment on lines +891 to +904
const style = { margin: 10, opacity: 0.5 };
const originalMargin = style.margin;
const originalOpacity = style.opacity;

render(<div style={style} />, scratch);

expect(style.margin).to.equal(
originalMargin,
'style.margin should not be mutated'
);
expect(style.opacity).to.equal(
originalOpacity,
'style.opacity should not be mutated'
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's simplify this:

Suggested change
const style = { margin: 10, opacity: 0.5 };
const originalMargin = style.margin;
const originalOpacity = style.opacity;
render(<div style={style} />, scratch);
expect(style.margin).to.equal(
originalMargin,
'style.margin should not be mutated'
);
expect(style.opacity).to.equal(
originalOpacity,
'style.opacity should not be mutated'
);
const style = { margin: 10, opacity: 0.5 };
render(<div style={style} />, scratch);
expect(style).to.equal({ margin: 10, opacity: 0.5 });

Comment thread compat/src/render.js
for (let key in value) {
if (typeof value[key] === 'number' && !IS_NON_DIMENSIONAL.test(key)) {
if (!cloned) {
value = assign({}, value);

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The clone is now lazy — only runs the first time a numeric value actually needs appended, so style objects with no such properties pay zero clone cost.

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.

3 participants