fix: use toString and prototype instead of constructor for type checking
#9
Conversation
🦋 Changeset detectedLatest commit: 381208a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
WalkthroughThis update replaces direct constructor comparisons with type checks using Changes
Sequence Diagram(s)sequenceDiagram
participant Test
participant VM
participant Hash
Test->>Hash: hash(obj1)
Test->>VM: runInNewContext
VM->>Hash: hash(obj2)
Hash->>Hash: isType/isPlainObject checks (toString/prototype)
Hash-->>Test: return hash for obj1 and obj2
Test->>Test: assert hash(obj1) === hash(obj2)
Assessment against linked issues
Suggested labels
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/index.tsOops! Something went wrong! :( ESLint: 9.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.js tests/unit.spec.tsOops! Something went wrong! :( ESLint: 9.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.js ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 48 59 +11
Branches 15 19 +4
=========================================
+ Hits 48 59 +11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull Request Overview
This PR replaces constructor-based type checks with Object.prototype.toString for more reliable type detection, adds a helper to recognize plain objects, and includes a cross-realm hashing test to ensure consistent behavior across VM contexts. It also updates documentation and bundle-size limits to account for these changes.
- Introduce
isTypeandisPlainObjecthelpers and updatestableHashto use them - Add a unit test for hashing objects across realms
- Update README and size-limit to reflect cross-realm support and new bundle size
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/index.ts | Replace constructor checks with isType and isPlainObject helpers |
| tests/unit.spec.ts | Add across realms test using runInNewContext |
| README.md | Note cross-realm support and update bundle size |
| .size-limit.json | Bump size limit from 460B to 540B |
Comments suppressed due to low confidence (4)
src/index.ts:16
- Consider adding a unit test for plain objects with a null prototype (e.g.,
Object.create(null)) to ensureisPlainObjecthandles this case correctly.
const isPlainObject = (val: object) => {
tests/unit.spec.ts:368
- Add a cross-realm RegExp hashing test to verify that regular expressions from different contexts produce consistent hashes.
test('across realms', () => {
src/index.ts:12
- Add a brief JSDoc comment above
isTypeexplaining that it checks the internal[[Class]]tag of an object for more reliable type detection.
const isType = (arg: unknown, type: string): boolean =>
src/index.ts:12
- [nitpick] The helper name
isTypeis quite generic; consider renaming it to something more descriptive likehasTypeTagorisObjectTypeTagfor clarity.
const isType = (arg: unknown, type: string): boolean =>
commit: |
size-limit report 📦
|
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 600adfa in 50 seconds. Click for details.
- Reviewed
129lines of code in4files - Skipped
0files when reviewing. - Skipped posting
9draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .size-limit.json:4
- Draft comment:
Updated bundle size limit to 540B to reflect the increased bundle size from added cross‐realm support. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. README.md:16
- Draft comment:
Documentation updated to show the new size (540B) and mention cross‐realm support. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. src/index.ts:10
- Draft comment:
Destructuring Object.prototype.toString ensures robust type checking, even if a user overrides toString. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. src/index.ts:12
- Draft comment:
Introducing the isType helper provides consistent type checking (via Object.prototype.toString) across realms. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. src/index.ts:36
- Draft comment:
Replacing the constructor-based Date check with isType improves cross-realm compatibility. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. src/index.ts:38
- Draft comment:
Using '!isType(arg, "RegExp")' instead of a constructor check makes the RegExp detection more robust across different realms. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. src/index.ts:52
- Draft comment:
Switching to Array.isArray for array detection is a best practice compared to constructor checks. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. src/index.ts:60
- Draft comment:
Replacing the constructor check for plain objects with the isPlainObject helper boosts robustness, especially for cross-realm objects. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. tests/unit.spec.ts:368
- Draft comment:
Added a cross‐realm test to verify that hashing yields consistent results across different VM contexts. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_G8iK9LHlGSdx9ZNC
You can customize 
|
Deploy preview for stable-hash-x ready! ✅ Preview Built with commit 381208a. |
📊 Package size report 3%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 381208a in 1 minute and 0 seconds. Click for details.
- Reviewed
147lines of code in6files - Skipped
0files when reviewing. - Skipped posting
9draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .prettierignore:2
- Draft comment:
Ensure that ignoring '/public/site.webmanifest' is intentional. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. .size-limit.json:4
- Draft comment:
Bump to 540B: verify that the new size limit accurately reflects the updated output. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. README.md:16
- Draft comment:
Documentation now shows 540b and mentions cross‐realm support. Confirm that this accurately reflects the library's behavior. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. src/index.ts:12
- Draft comment:
Good use of Object.prototype.toString via the isType helper for robust type checking across different realms. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. src/index.ts:36
- Draft comment:
Replacing constructor-based checks with isType for Date and RegExp improves cross-realm compatibility. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. src/index.ts:52
- Draft comment:
Using Array.isArray instead of a constructor check is a best practice for array detection. - Reason this comment was not posted:
Confidence changes req 8000 uired:0%<= threshold50%None
7. src/index.ts:60
- Draft comment:
Using isPlainObject for plain object detection is a solid improvement over direct constructor comparison. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. src/index.ts:65
- Draft comment:
Iterating over sorted keys by popping from the array works but may be less intuitive; consider a forward iteration for clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
9. tests/unit.spec.ts:368
- Draft comment:
Nice addition: the 'across realms' test effectively validates that cross-realm objects produce the same hash. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_rojaKbcPWV74bSEj
You can customize
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.prettierignore (1)
2-2: Consider if this change belongs in this PR.Adding
/public/site.webmanifestto the ignore list seems unrelated to the main PR objective of cross-realm object support. While harmless, consider whether this should be in a separate commit or PR focused on project configuration.src/index.ts (1)
9-10: Address variable shadowing warning.The static analysis tool correctly identifies that extracting
toStringshadows the global property. Consider renaming to avoid confusion.Apply this diff to address the shadowing:
-// eslint-disable-next-line @typescript-eslint/unbound-method -const { toString } = Object.prototype // type-coverage:ignore-line +// eslint-disable-next-line @typescript-eslint/unbound-method +const { toString: objectToString } = Object.prototype // type-coverage:ignore-lineThen update the usage in the
isTypefunction:const isType = (arg: unknown, type: string): boolean => - toString.call(arg) === `[object ${type}]` + objectToString.call(arg) === `[object ${type}]`README.md (1)
16-16: LGTM! Documentation accurately reflects the improvements.The updated description correctly mentions both the new size (540B) and the added cross-realm object support, which aligns perfectly with the technical changes implemented.
Optional style improvement based on static analysis:
-A tiny and fast (540b <sup>[unpkg](https://unpkg.com/stable-hash-x@latest/lib/index.js)</sup>) lib for "stably hashing" a JavaScript value, works with cross-realm objects. Originally created for [SWR](https://github.com/vercel/swr) by [Shu Ding][] at [`stable-hash`](https://github.com/shuding/stable-hash), we forked it because the original one is a bit out of maintenance for a long time. +A tiny and fast (540b <sup>[unpkg](https://unpkg.com/stable-hash-x@latest/lib/index.js)</sup>) lib for "stably hashing" a JavaScript value, works with cross-realm objects. Created for [SWR](https://github.com/vercel/swr) by [Shu Ding][] at [`stable-hash`](https://github.com/shuding/stable-hash), we forked it because the original one is a bit out of maintenance for a long time.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/easy-boxes-shine.md(1 hunks).prettierignore(1 hunks).size-limit.json(1 hunks)README.md(1 hunks)src/index.ts(4 hunks)tests/unit.spec.ts(2 hunks)
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~16-~16: This phrase is redundant. Consider writing “created”.
Context: ... value, works with cross-realm objects. Originally created for [SWR](https://github.com/vercel/swr...
(ORIGINALLY_CREATED)
🪛 Biome (1.9.4)
src/index.ts
[error] 10-10: Do not shadow the global "toString" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Lint and Test with Node.js 20 on macos-latest
- GitHub Check: Lint and Test with Node.js 22 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 on macos-latest
- GitHub Check: Lint and Test with Node.js 24 on windows-latest
🔇 Additional comments (8)
.size-limit.json (1)
4-4: LGTM! Size increase reflects added cross-realm functionality.The 80-byte increase (from 460B to 540B) is reasonable for the added cross-realm object support implemented through the new
isTypeandisPlainObjectutility functions.src/index.ts (4)
12-13: LGTM! Excellent cross-realm type detection.The
isTypefunction usingObject.prototype.toString.call()is the correct approach for cross-realm type detection. This will consistently return type strings like "[object Date]" regardless of the JavaScript realm.
15-23: LGTM! Robust plain object detection.The
isPlainObjectimplementation correctly handles cross-realm scenarios by checking prototype chains rather than constructors. The logic properly identifies:
- Objects with null prototype (
Object.create(null))- Objects with
Object.prototypeas prototype- Objects whose prototype's prototype is null (e.g.,
Object.create(Object.prototype))This approach works consistently across JavaScript realms.
36-38: LGTM! Proper replacement of constructor-based checks.The replacement of constructor-based date and regex checks with
isTypecalls ensures consistent behavior across JavaScript realms. The logic correctly excludes dates and regexes from the object hashing path.
52-60: LGTM! Array and plain object detection improved.Replacing
Array.isArray()and constructor-based object checks with the new utility functions ensures consistent behavior across realms. TheisPlainObjectcheck properly identifies objects that should have their keys sorted for stable hashing..changeset/easy-boxes-shine.md (1)
1-6: LGTM! Properly documented changeset.The changeset correctly categorizes this as a patch-level fix and accurately describes the core change from constructor-based to toString/prototype-based type checking.
tests/unit.spec.ts (2)
3-3: Good use of modern Node.js import syntax.The import uses the recommended
node:prefix for built-in modules, which is a best practice for modern Node.js applications.
368-383: Excellent test coverage for cross-realm functionality.This test effectively validates the core improvement described in the PR objectives. The test design is comprehensive:
- Tests multiple object types (number, Date, RegExp, Symbol) across realms
- Correctly verifies that objects from different realms are not strictly equal
- Validates that the new hashing implementation produces consistent hashes across realms
The Symbol handling is particularly noteworthy - even though
Symbol('test')creates different symbol instances in each realm, the test expects consistent hashing, which suggests the implementation now hashes symbols by their description rather than identity.
close shuding/stable-hash#22
Important
Replace
constructorwithtoStringandprototypefor type checking instable-hash-x, adding cross-realm support and updating documentation and configurations.constructorwithtoStringandprototypefor type checking insrc/index.ts.unit.spec.ts.README.mdto 540B to reflect new functionality..size-limit.jsonto 540B./public/site.webmanifestto.prettierignore.This description was created by
for 381208a. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores