Skip to content
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.

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

fix: avoid isObject for internal use #18

Merged
merged 4 commits into from
Jun 25, 2024
Merged

fix: avoid isObject for internal use #18

merged 4 commits into from
Jun 25, 2024

Conversation

aleclarson
Copy link
Member

@aleclarson aleclarson commented Jun 24, 2024

Description

Using isObject internally is hostile to any use case in need of Object.create(null), so we'll use isPlainObject instead.

Affected functions include:

  • assign
  • crush
  • keys

What does this mean?

When an object created with Object.create(null) is nested in the argument to assign, crush, or keys, it will be treated as if it was created with {}. In the case of assign, any time such an object is assigned to, the resulting clone of the object will have a null prototype as expected.

Checklist

  • Changes are covered by tests if behavior has been changed or added
  • Tests have 100% coverage
  • If code changes were made, the documentation (in the /docs directory) has been updated

Resolves

Resolves sodiray/radash#409

src/object.ts Outdated
@@ -275,10 +281,10 @@ export const assign = <X extends Record<string | symbol | number, any>>(
override: X
): X => {
if (!initial || !override) return initial ?? override ?? {}
const merged = { ...initial }
const merged = cloneObject(initial)
Copy link
Member Author

@aleclarson aleclarson Jun 24, 2024

Choose a reason for hiding this comment

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

Point of discussion

This line changes the behavior of assign in two subtle ways:

  1. It copies non-enumerable properties into the new object.
  2. It copies the prototype into the new object, potentially calling a class constructor.

For these reasons, this is a breaking change, BUT the behavior of assign is so woefully underspecified that we could probably get away with publishing this in a minor version.

The question is, “what's the intuitive behavior?” One might argue it's whatever aligns with Object.assign, as this is basically a recursive version of that.

The best course of action is likely to implement a simpler variant of cloneObject that does { ...initial } unless an object created with Object.create(null) is found, in which case, a mixture of Object.create(null) & Object.assign is performed.

Copy link
Member Author

Choose a reason for hiding this comment

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

After writing that out, the answer is clear that what I described in the last paragraph is how it should work.

@aleclarson aleclarson merged commit 12d6fd0 into main Jun 25, 2024
4 checks passed
aleclarson added a commit that referenced this pull request Jun 25, 2024
* docs: add warning about `isObject` and `Object.create(null)`

* fix: use isPlainObject internally

* test: assign/keys with Object.create(null)

* fix: preserve null prototype in assign result
@aleclarson aleclarson added the upstream fix Fixes a bug that existed in Radash label Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
upstream fix Fixes a bug that existed in Radash
Projects
None yet
Development

Successfully merging this pull request may close these issues.

isObject should not be used by other functions in radash
1 participant