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: do not deep convert buffers #126

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dword-design
Copy link

@dword-design dword-design commented Jun 22, 2024

Buffers are traversed recursively but they shouldn't. I think it's also relevant for other related packages.

index.js Outdated
@@ -20,7 +21,8 @@ const isObject = value =>
&& value !== null
&& !(value instanceof RegExp)
&& !(value instanceof Error)
&& !(value instanceof Date);
&& !(value instanceof Date)
&& !(value instanceof Buffer);
Copy link
Owner

Choose a reason for hiding this comment

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

Actually, not just Buffer. It should detect any TypedArray

Copy link
Author

Choose a reason for hiding this comment

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

@sindresorhus True. I was actually thinking if this lib should not convert any class instances but just arrays and plain objects. I've adjusted the implementation. If we want to check typed arrays, we could do ArrayBuffer.isView(), which is true for TypedArrays + DataViews (which we probably also want to exclude).

&& !(value instanceof RegExp)
&& !(value instanceof Error)
&& !(value instanceof Date);
&& (Array.isArray(value) || Object.getPrototypeOf(value) === Object.prototype);
Copy link
Owner

Choose a reason for hiding this comment

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

Move Object.getPrototypeOf(value) === Object.prototype out into a const function to improve readability by giving it a name.

Copy link
Owner

Choose a reason for hiding this comment

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

And I think the behavior should be documented.

@@ -18,9 +18,7 @@ const cache = new QuickLru({maxSize: 100_000});
const isObject = value =>
Copy link
Owner

Choose a reason for hiding this comment

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

The name here no longer makes any sense.

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.

2 participants