fix: prefer hasOwnProperty over hasOwn#1421
fix: prefer hasOwnProperty over hasOwn#1421alaycock-stripe wants to merge 1 commit intoopen-circle:mainfrom
Conversation
|
@alaycock-stripe is attempting to deploy a commit to the Open Circle Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughA single utility function in Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates Valibot’s object-key validation utility to avoid Object.hasOwn and instead use Object.prototype.hasOwnProperty.call, improving compatibility with older browsers and reducing the need for a polyfill.
Changes:
- Replaced
Object.hasOwn(object, key)withObject.prototype.hasOwnProperty.call(object, key)in_isValidObjectKey.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
How much of an impact it the bundle size of the polyfill? Another option for you is to copy our |
|
When using core-js to polyfill several bundles are getting a ~3kb bump. I haven't explored into why the impact is so significant. I'll look into patching, but was curious if this change would be appreciated upstream. Your tsconfig targets ES2020 but hasOwn landed in ES2022. |
This is a good argument. On the other side hasOwn should have around 97% coverage across browsers and no one has complained so far. I was using hasOwn instead of hasOwnProperty because the MDN docs recommended it. |
commit: |
|
I think I prefer to wait before making a decision. |
We are using Valibot for a project that requires significant browser backwards compatibility but I am unable to use some schemas because of
Object.hasOwn. While this is possible to polyfill, this small change would allow us to drop our polyfill, reducing our bundle size. The tradeoff is a small reduction in readability for significantly better browser compatibility.This change is similar to an existing polyfill, except it relies on types rather than a throwing an exception.
The change should have no runtime impact, all tests continue to pass.
Summary by CodeRabbit