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

Avoid unnecesary prototype loop #3786

Closed
wants to merge 1 commit into from

Conversation

WebReflection
Copy link

Background

People at redux-toolkit copy/pasted this utility in there, and they are not welcoming a PR because of that, but the while loop in here is completely unnecessary, as the comparison is made on the first proto only.

Improvements

  • O(1) instead of O(n)
  • safe fallback for Object.create(null) cases
  • fast lane for new Class() derived objects
  • better performance
  • smaller code size

As this change has zero downsides, unless I am missing some explicit reason to while loop instead of never using getPrototypeOf more than twice, I hope it'll get in, so that others might copy and paste this new version.

Best Regards.

Thanks for the PR!

To better assist you, please select the type of PR you want to create.

Click the "Preview" tab above, and click on the link for the PR type:

## Background

People at redux-toolkit copy/pasted this utility in there, and [they are not welcoming a PR](reduxjs/redux-toolkit#581) because of that, but the `while` loop in here is completely unnecessary, as the comparison is made on the first `proto` only.

## Improvements

  * _O(1)_ instead of _O(n)_
  * safe fallback for `Object.create(null)` cases
  * fast lane for `new Class()` derived objects
  * better performance
  * smaller code size

As this change has zero downsides, unless I am missing some explicit reason to `while` loop instead of never using `getPrototypeOf` more than twice, I hope it'll get in, so that others might copy and paste this new version.

Best Regards.
@netlify
Copy link

netlify bot commented May 25, 2020

Deploy preview for redux-docs ready!

Built with commit 53c59b0

https://deploy-preview-3786--redux-docs.netlify.app

@timdorr
Copy link
Member

timdorr commented May 25, 2020

Same reasoning here as your linked PR. You can read the background in #2599.

@timdorr timdorr closed this May 25, 2020
@WebReflection
Copy link
Author

WebReflection commented May 25, 2020

If ever interested in actually improving this little part of the equation, I've fixed all Travis CI related issues. It would've picked 'em up if not closed.

/**
 * @param obj The object to inspect.
 * @returns True if the argument appears to be a plain object.
 */
export default function isPlainObject(obj: any): boolean {
  return (
    typeof obj === 'object' &&
    obj !== null &&
    Object.getPrototypeOf(Object.getPrototypeOf(obj) || {}) === null
  )
}

Best Regards.

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