Skip to content

Fix #1048: Allow rest props to override lazyProps data properties #1071

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
erikras-dinesh-agent wants to merge 1 commit into from
+72 −9
Closed

Fix #1048: Allow rest props to override lazyProps data properties #1071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions src/renderComponent.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,3 +142,56 @@ describe("renderComponent", () => {
expect(result.props.active).toBe("getter-value");
});
});

describe("renderComponent - Issue #1048", () => {
it("should allow rest props to override lazyProps data properties", () => {
const Component = () => null;
const lazyProps = {
input: {
value: "",
onChange: () => {},
},
};

const customInput = {
value: "foo",
onChange: () => {},
};

const result = renderComponent(
{ component: Component, input: customInput },
lazyProps,
"Field",
);

expect(result.type).toBe(Component);
// The custom input prop should override the lazyProps input
expect(result.props.input).toBe(customInput);
expect(result.props.input.value).toBe("foo");
});

it("should still protect getter-only properties from being overwritten", () => {
const Component = () => null;
const lazyProps = {
data: { regular: "value" },
};

// Add a getter-only property
Object.defineProperty(lazyProps, "active", {
get: () => "getter-value",
enumerable: true,
});

const result = renderComponent(
{ component: Component, active: "override-attempt", custom: "prop" },
lazyProps,
"Field",
);

expect(result.type).toBe(Component);
// Getter-only property should NOT be overridden
expect(result.props.active).toBe("getter-value");
// Regular data property should be overridable
expect(result.props.custom).toBe("prop");
});
});
28 changes: 19 additions & 9 deletions src/renderComponent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,18 @@ export default function renderComponent<T>(
if (component) {
// FIX: Don't use Object.assign which tries to overwrite getters
// Instead, create a new object with lazyProps descriptors first,
// then add non-conflicting properties from rest
// then add properties from rest (but don't overwrite getter-only properties)
const result = {} as any;
Object.defineProperties(result, Object.getOwnPropertyDescriptors(lazyProps));
const restDescriptors = Object.getOwnPropertyDescriptors(rest);
for (const key in restDescriptors) {
if (!(key in result)) {
Object.defineProperty(result, key, restDescriptors[key]);
const existingDescriptor = Object.getOwnPropertyDescriptor(result, key);
// Skip getter-only properties (these would throw an error if we tried to overwrite them)
if (existingDescriptor && existingDescriptor.get && !existingDescriptor.set) {
continue;
}
// For everything else, allow rest to override lazyProps
Object.defineProperty(result, key, restDescriptors[key]);
Comment on lines +15 to +26
Copy link

@coderabbitai coderabbitai bot Feb 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

JavaScript object rest spread syntax symbol keys enumerable properties

💡 Result:

  • Object spread ({ ...obj }) copies only the source’s own, enumerable properties into the new object (it’s a shallow copy; it does not copy non-enumerable properties or anything from the prototype chain). Enumerable Symbol-keyed properties are included (non-enumerable symbol properties are not). [1][2][3]
  • Object rest in destructuring (const { a, ...rest } = obj) collects the remaining own, enumerable property keys (i.e., what’s left after the picked properties), and this likewise includes enumerable symbol keys. [4][2]

Example:

const s1 = Symbol('enum');
const s2 = Symbol('nonenum');

const o = { a: 1, [s1]: 2 };
Object.defineProperty(o, s2, { value: 3, enumerable: false });

const spread = { ...o };      // has a and s1, not s2
const { a, ...rest } = o;     // rest has s1, not s2

(Enumerability of symbol properties matters specifically for Object.assign() and object spread.) [3][2]

Sources: [1] MDN Spread syntax, [2] MDN Enumerability and ownership of properties, [3] MDN propertyIsEnumerable(), [4] MDN Destructuring assignment (rest in object destructuring).

🌐 Web query:

Object.assign copy symbol keys properties

💡 Result:

Object.assign(target, ...sources) copies own, enumerable properties from each source onto target, and it copies both string-keyed and symbol-keyed properties (i.e., enumerable Symbol properties are included). It does not copy properties from the prototype chain or any non-enumerable properties. It performs assignment via [[Get]] on the source and [[Set]] on the target (so getters/setters can run). [1] [2]

🌐 Web query:

for..in loop symbol keys iteration JavaScript

💡 Result:

for...in does not iterate Symbol-keyed properties—it only visits enumerable string property keys (and it also ignores Symbol keys even if they’re enumerable). [1]

To iterate Symbol keys, use one of these:

Only Symbol keys (own properties)

for (const sym of Object.getOwnPropertySymbols(obj)) {
  console.log(sym, obj[sym]);
}

Object.getOwnPropertySymbols() returns an array of an object’s own Symbol properties. [2]

All own keys (strings + symbols; enumerable + non-enumerable)

for (const key of Reflect.ownKeys(obj)) {
  console.log(key, obj[key]);
}

Reflect.ownKeys() returns all own property keys, including both strings and symbols. [3]

MDN also explicitly notes Symbols aren’t enumerable in for...in, and points to Object.getOwnPropertySymbols() as the way to retrieve them. [4]

Symbol‑keyed rest props are dropped with for..in.
for..in skips symbol keys entirely, which diverges from object‑rest / Object.assign semantics that include enumerable symbols. Consider iterating with Reflect.ownKeys() instead and apply the same change to the other two merge loops.

🔧 Suggested fix (apply similarly in the other two loops) 
-    const restDescriptors = Object.getOwnPropertyDescriptors(rest);
-    for (const key in restDescriptors) {
+    const restDescriptors =
+      Object.getOwnPropertyDescriptors(rest) as Record<PropertyKey, PropertyDescriptor>;
+    for (const key of Reflect.ownKeys(restDescriptors)) {
       const existingDescriptor = Object.getOwnPropertyDescriptor(result, key);
       // Skip getter-only properties (these would throw an error if we tried to overwrite them)
       if (existingDescriptor && existingDescriptor.get && !existingDescriptor.set) {
         continue;
       }
       // For everything else, allow rest to override lazyProps
       Object.defineProperty(result, key, restDescriptors[key]);
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// then add properties from rest (but don't overwrite getter-only properties)
const result = {} as any;
Object.defineProperties(result, Object.getOwnPropertyDescriptors(lazyProps));
const restDescriptors = Object.getOwnPropertyDescriptors(rest);
for (const key in restDescriptors) {
if (!(key in result)) {
Object.defineProperty(result, key, restDescriptors[key]);
const existingDescriptor = Object.getOwnPropertyDescriptor(result, key);
// Skip getter-only properties (these would throw an error if we tried to overwrite them)
if (existingDescriptor && existingDescriptor.get && !existingDescriptor.set) {
continue;
}
// For everything else, allow rest to override lazyProps
Object.defineProperty(result, key, restDescriptors[key]);
// then add properties from rest (but don't overwrite getter-only properties)
const result = {} as any;
Object.defineProperties(result, Object.getOwnPropertyDescriptors(lazyProps));
const restDescriptors =
Object.getOwnPropertyDescriptors(rest) as Record<PropertyKey, PropertyDescriptor>;
for (const key of Reflect.ownKeys(restDescriptors)) {
const existingDescriptor = Object.getOwnPropertyDescriptor(result, key);
// Skip getter-only properties (these would throw an error if we tried to overwrite them)
if (existingDescriptor && existingDescriptor.get && !existingDescriptor.set) {
continue;
}
// For everything else, allow rest to override lazyProps
Object.defineProperty(result, key, restDescriptors[key]);
}
🤖 Prompt for AI Agents 
In `@src/renderComponent.ts` around lines 15 - 26, The merge loop that copies
descriptors from rest to result uses "for (const key in restDescriptors)" which
omits symbol keys; update the loop in renderComponent.ts to iterate over
Reflect.ownKeys(restDescriptors) (or Object.getOwnPropertyNames +
Object.getOwnPropertySymbols) so symbol-keyed props are preserved, and inside
the loop use the same existingDescriptor check (existingDescriptor &&
existingDescriptor.get && !existingDescriptor.set) and
Object.defineProperty(result, key, restDescriptors[key]) as before; apply the
same replacement (using Reflect.ownKeys) to the other two similar merge loops in
this file to ensure enumerable symbol properties are not dropped.

}
result.children = children;
result.render = render;
Expand All @@ -31,12 +35,15 @@ export default function renderComponent<T>(
result,
Object.getOwnPropertyDescriptors(lazyProps),
);
// Only add properties from rest that don't already exist
// Add properties from rest (but don't overwrite getter-only properties)
const restDescriptors = Object.getOwnPropertyDescriptors(rest);
for (const key in restDescriptors) {
if (!(key in (result as any))) {
Object.defineProperty(result as any, key, restDescriptors[key]);
const existingDescriptor = Object.getOwnPropertyDescriptor(result as any, key);
// Skip getter-only properties
if (existingDescriptor && existingDescriptor.get && !existingDescriptor.set) {
continue;
}
Object.defineProperty(result as any, key, restDescriptors[key]);
}
if (children !== undefined) {
(result as any).children = children;
Expand All @@ -50,12 +57,15 @@ export default function renderComponent<T>(
}
const result = {} as T;
Object.defineProperties(result, Object.getOwnPropertyDescriptors(lazyProps));
// Only add properties from rest that don't already exist
// Add properties from rest (but don't overwrite getter-only properties)
const restDescriptors = Object.getOwnPropertyDescriptors(rest);
for (const key in restDescriptors) {
if (!(key in (result as any))) {
Object.defineProperty(result as any, key, restDescriptors[key]);
const existingDescriptor = Object.getOwnPropertyDescriptor(result as any, key);
// Skip getter-only properties
if (existingDescriptor && existingDescriptor.get && !existingDescriptor.set) {
continue;
}
Object.defineProperty(result as any, key, restDescriptors[key]);
}
return children(result);
}
Loading