refactor: add a new crossRealm option for performance
#11
Conversation
🦋 Changeset detectedLatest commit: 46ff91a 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 |
WalkthroughThis update introduces a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant stableHash
participant isType
User->>stableHash: stableHash(obj, crossRealm?)
alt crossRealm is true
stableHash->>isType: Check type using toString tag
else crossRealm is false or undefined
stableHash->>stableHash: Check type using constructor
end
stableHash-->>User: Return hash string
Possibly related PRs
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
benchmark.jsOops! Something went wrong! :( ESLint: 9.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.js 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 comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
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. |
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 |
1 similar comment
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 |
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 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 59 69 +10
Branches 19 26 +7
=========================================
+ Hits 59 69 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
commit: |
size-limit report 📦
|
📊 Package size report 10%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
|
Deploy preview for stable-hash-x ready! ✅ Preview Built with commit 46ff91a. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 9b11092 in 1 minute and 46 seconds. Click for details.
- Reviewed
282lines of code in6files - Skipped
1files when reviewing. - Skipped posting
4draft 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. src/index.ts:62
- Draft comment:
Recursive call for array elements does not propagate the crossRealm flag. Use stableHash(arg_[index], crossRealm) to ensure consistent behavior for nested arrays. - Reason this comment was not posted:
Comment was on unchanged code.
2. README.md:54
- Draft comment:
Consider clarifying in the documentation that enabling the crossRealm flag may impact performance and that it ensures cross-realm type checks (e.g., for Date, RegExp) across nested values. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. README.md:54
- Draft comment:
Typographical suggestion: Consider adding an article for clarity. For example, change "in cross-realm environment" to "in a cross-realm environment". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a very minor grammatical suggestion in a code comment. While technically correct, it doesn't materially impact understanding. The rules state not to make purely informative comments or comments that are obvious/unimportant. This seems to fall into that category. The grammar could potentially impact readability for non-native English speakers. Documentation quality is important. While documentation quality matters, this change is too minor to warrant a PR comment. The meaning is clear either way. Delete this comment as it's too minor and doesn't affect code functionality or meaningful documentation clarity.
4. benchmark.js:50
- Draft comment:
Typographical error: The comment reads "Ensure the input is flatten", which might be intended as "Ensure the input is flattened". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_6qyD1F5Lx5Z2Cwlh
You can customize 
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 |
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/index.ts (1)
62-62: Fix missing crossRealm flag propagation in array processing.The recursive call to
stableHashin the array branch doesn't pass thecrossRealmflag, which will cause inconsistent behavior compared to the object branch.Apply this fix:
- result += stableHash(arg_[index]) + ',' + result += stableHash(arg_[index], crossRealm) + ','
🧹 Nitpick comments (1)
src/index.ts (1)
36-36: Consider renaming the constructor variable to avoid shadowing.The static analysis tool correctly flags that
constructorshadows the global constructor property, which could lead to confusion.Apply this fix:
- const constructor = arg?.constructor + const ctor = arg?.constructorThen update the conditional checks accordingly:
- const isDate = crossRealm ? isType(arg, 'Date') : constructor === Date + const isDate = crossRealm ? isType(arg, 'Date') : ctor === DateAnd similarly for the other constructor checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
.changeset/four-beds-visit.md(1 hunks)README.md(3 hunks)benchmark.js(3 hunks)benchmark.txt(1 hunks)package.json(2 hunks)src/index.ts(3 hunks)tests/unit.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
benchmark.js (1)
src/index.ts (2)
stableHash(34-94)stableHash(96-96)
🪛 Biome (1.9.4)
src/index.ts
[error] 36-36: Do not shadow the global "constructor" 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 (12)
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on ubuntu-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 20 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-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 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 on windows-latest
🔇 Additional comments (11)
package.json (1)
37-37: LGTM! Clean benchmarking refactor.The simplification of the benchmark script and the switch from
mitatatotinybenchlooks good. Removingsudois also a security improvement.Also applies to: 86-87
.changeset/four-beds-visit.md (1)
1-6: LGTM! Proper changeset documentation.The changeset correctly documents this as a minor version change with an appropriate description of the crossRealm feature addition.
tests/unit.spec.ts (1)
382-382: LGTM! Correct usage of the crossRealm option.The test properly demonstrates the crossRealm feature by enabling it for cross-realm object comparison. This validates that objects from different JavaScript realms can be hashed consistently when the option is enabled.
src/index.ts (1)
34-34: LGTM! Well-implemented crossRealm feature.The crossRealm parameter implementation is excellent:
- Provides performance benefit by defaulting to constructor-based checks
- Falls back to
toString.call()for cross-realm scenarios when needed- Properly handles Date, RegExp, Array, and Object types
- Maintains consistent API with optional parameter
The conditional type detection logic is sound and addresses the core challenge of cross-realm object identification.
Also applies to: 37-43, 57-57, 65-65
README.md (2)
34-34: LGTM! Excellent documentation of the crossRealm feature.The documentation clearly explains:
- When to use the crossRealm option
- Performance implications (disabled by default)
- Practical example using Node.js
vmmodule- Updated usage instructions
The cross-realm example effectively demonstrates the feature's value for handling objects from different JavaScript realms.
Also applies to: 54-55, 159-179
184-192: LGTM! Clear benchmark presentation.The tabular format makes it easy to compare performance across different hashing implementations. Good to see
stable-hash-xperforming competitively with the originalstable-hash.benchmark.txt (1)
1-8: Well-formatted benchmark results with clear performance insights.The tabular format effectively presents the performance comparison between the four hashing implementations. The results show that
stable-hash-xandstable-hashperform similarly (both ~7800ns latency), which validates the new implementation's performance characteristics.benchmark.js (4)
9-12: Import changes look correct for the benchmarking refactor.The imports properly set up the comparison between the external
stable-hashpackage and the local implementation. The naming convention clearly distinguishes betweenstableHash(external) andhash(local).
46-46: Comment accurately reflects the updated benchmark scope.The comment correctly includes all four implementations being benchmarked.
91-101: New hash function follows consistent pattern.The
getHashFourfunction correctly implements the same hashing pipeline as the other functions: flattening → hashing → SHA-512 → base64 URL encoding. This ensures fair performance comparison across all implementations.
103-113: Benchmarking refactor properly implements tinybench API.The migration from mitata to tinybench is correctly implemented with proper async/await handling and clear result presentation using
console.table(). The benchmark setup ensures all implementations are tested with the same payload for fair comparison.
JounQin
force-pushed
the
refactor/cross-realm
branch
2 times, most recently
from
256da76 to
793699d
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 256da76 in 1 minute and 53 seconds. Click for details.
- Reviewed
315lines of code in8files - Skipped
1files when reviewing. - Skipped posting
15draft 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. .changeset/four-beds-visit.md:5
- Draft comment:
Changeset message is clear and matches the PR title. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. .size-limit.json:4
- Draft comment:
Size limit updated from 540B to 600B seems intentional given the refactor. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. README.md:160
- Draft comment:
The added cross‐realm usage section is clear and provides a useful example. Consider briefly noting any performance trade-offs. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. benchmark.js:10
- Draft comment:
Refactoring the benchmark script to use tinybench improves clarity and output formatting. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
5. package.json:84
- Draft comment:
Dependency updates (removing mitata and adding stable-hash and tinybench) align with the new benchmark implementation. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. src/index.ts:37
- Draft comment:
Good use of the crossRealm flag: using isType for Date detection ensures compatibility across realms. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. src/index.ts:42
- Draft comment:
Consistent conditional check for RegExp using the crossRealm flag is clear and appropriate. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. src/index.ts:57
- Draft comment:
Using Array.isArray for cross-realm detection is a robust solution for arrays. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. src/index.ts:65
- Draft comment:
When crossRealm is true, plain object detection relies on isPlainObject. Ensure this works as expected for objects from different realms (their prototypes may differ); consider documenting any limitations. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%The comment is asking the author to ensure that a specific behavior works as expected, which violates the rule against asking the author to ensure behavior. However, it also provides a suggestion to document limitations, which could be useful. The comment is partially informative but also provides a specific suggestion.
10. tests/unit.spec.ts:382
- Draft comment:
The cross‐realm test now passes the true flag to hash, verifying the new option correctly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
11. README.md:35
- Draft comment:
Typographical error: The bullet for 'Benchmark' is missing the two-space indentation found in other list items. Consider updating it to maintain consistent formatting. - Reason this comment was not posted:
Comment was on unchanged code.
12. README.md:36
- Draft comment:
Typographical error: The bullet for 'Notes' is missing the two-space indentation for consistency with the rest of the list items. - Reason this comment was not posted:
Comment was on unchanged code.
13. README.md:37
- Draft comment:
Typographical error: The bullet for 'Sponsors and Backers' is missing the two-space indentation present in other list items. - Reason this comment was not posted:
Comment was on unchanged code.
14. README.md:54
- Draft comment:
Typographical suggestion: Consider adding an article for clarity. Instead of "if you're running in cross-realm environment", use "if you're running in a cross-realm environment". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a very minor grammatical suggestion that doesn't affect code functionality or documentation clarity. The meaning is perfectly clear without the article. PR comments should focus on more substantial issues. Grammar improvements can sometimes be important for documentation quality. Poor grammar could reflect badly on the project. While grammar is important, this is too minor of an issue to warrant a PR comment. The current phrasing is acceptable technical writing. Delete this comment as it's too minor and doesn't affect understanding of the documentation.
15. benchmark.js:50
- Draft comment:
Typographical issue: The phrase "Ensure the input is flatten" appears to have a grammatical error. Consider changing "flatten" to "flattened". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_ksXFpHOd83DDIubS
You can customize
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 793699d in 1 minute and 49 seconds. Click for details.
- Reviewed
315lines of code in8files - Skipped
1files when reviewing. - Skipped posting
7draft 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. src/index.ts:37
- Draft comment:
Using the crossRealm flag to determine Date type is clever. Note that for non–crossRealm mode, using 'constructor === Date' may not cover objects with a null prototype. If this trade‐off is intentional, please document it. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment raises a valid technical point about null prototype objects. However, looking at the code's purpose (stable hashing) and implementation, this edge case seems extremely unlikely to matter in practice. The code already handles null prototypes in isPlainObject, suggesting the author is aware of such cases. The comment is more speculative than pointing out a clear issue. I might be underestimating the importance of handling null prototype objects consistently throughout the code. The comment could be highlighting a real inconsistency in the codebase's approach to prototype handling. While consistency is valuable, the practical impact of this edge case is minimal. The code's main purpose is hashing, and the current implementation handles the common cases correctly. Delete the comment as it's more speculative than pointing out a clear issue, and asks for documentation of an intentional trade-off without strong evidence that it's problematic.
2. src/index.ts:57
- Draft comment:
Good job propagating the crossRealm flag in the recursive call for array elements. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. README.md:159
- Draft comment:
The new cross‐realm example is clear and useful. Consider emphasizing that the crossRealm flag is disabled by default for performance reasons. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. benchmark.js:95
- Draft comment:
Note that getHashFour (benchmarking 'stable-hash') does not pass the crossRealm flag. If comparing similar behavior, consider adding a benchmark that enables the new crossRealm option. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. package.json:37
- Draft comment:
The benchmark script has been simplified by removing 'sudo env NO_COLOR=1'. Confirm that this change doesn't affect CI output if colored logs are needed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. tests/unit.spec.ts:382
- Draft comment:
The cross‐realm test has been updated correctly to use the crossRealm flag (true). - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. benchmark.js:50
- Draft comment:
Typo: "Ensure the input is flatten" might be better written as "Ensure the input is flattened." - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_SieRdwMBpfMx5yZn
You can customize
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/index.ts (1)
36-36: Consider renaming theconstructorvariable to avoid shadowing the global property.The static analysis tool correctly identifies that naming a variable
constructorshadows the globalconstructorproperty, which can cause confusion.- const constructor = arg?.constructor + const argConstructor = arg?.constructorThen update the references throughout the function:
- const isDate = crossRealm ? isType(arg, 'Date') : constructor === Date + const isDate = crossRealm ? isType(arg, 'Date') : argConstructor === Date - !(crossRealm ? isType(arg, 'RegExp') : constructor === RegExp) + !(crossRealm ? isType(arg, 'RegExp') : argConstructor === RegExp) - if (crossRealm ? Array.isArray(arg) : constructor === Array) { + if (crossRealm ? Array.isArray(arg) : argConstructor === Array) { - } else if (crossRealm ? isPlainObject(arg_) : constructor === Object) { + } else if (crossRealm ? isPlainObject(arg_) : argConstructor === Object) {README.md (1)
16-16: Minor style suggestion: Consider simplifying the phrase.The documentation is well-written and accurately reflects the new crossRealm feature.
If you want to address the style hint, you could simplify:
-A tiny and fast (600b <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 (600b <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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
.changeset/four-beds-visit.md(1 hunks).size-limit.json(1 hunks)README.md(4 hunks)benchmark.js(3 hunks)benchmark.txt(1 hunks)package.json(2 hunks)src/index.ts(3 hunks)tests/unit.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .size-limit.json
🚧 Files skipped from review as they are similar to previous changes (4)
- .changeset/four-beds-visit.md
- package.json
- tests/unit.spec.ts
- benchmark.js
🧰 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] 36-36: Do not shadow the global "constructor" 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 (17)
- GitHub Check: autofix
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 on macos-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on macos-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 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on ubuntu-latest
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
src/index.ts (1)
34-34: Excellent implementation of the crossRealm feature!The crossRealm parameter is properly implemented with correct type checking logic and recursive parameter passing. This addresses the previous review comment about missing crossRealm in array branch recursive calls.
Also applies to: 57-57, 62-62, 65-65, 74-74
README.md (1)
34-34: Excellent documentation of the crossRealm feature!The cross-realm section provides a clear, practical example using Node.js vm module that demonstrates the feature's purpose and usage effectively.
Also applies to: 159-179
benchmark.txt (1)
1-9: Clean benchmark results presentation.The new table format provides a clear comparison of performance metrics across different hashing implementations.
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a crossRealm option to the stableHash function to enable consistent hashing of objects from different JavaScript realms, updates documentation and benchmarks to reflect the new feature, and adds tests to cover cross-realm scenarios.
- Add
crossRealmboolean parameter tostableHashand adjust type checks (Date/RegExp/Array/Object) - Update README with cross-realm usage example and bump documented bundle size
- Refactor
benchmark.jsto usetinybench, compare against the upstreamstable-hash, and updatebenchmark.txt
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit.spec.ts | Update test to assert hashing with the crossRealm flag |
| src/index.ts | Add crossRealm parameter and adjust type-detection branches |
| package.json | Remove sudo, bump dependencies, add tinybench, bump size limit |
| benchmark.js | Migrate from mitata to tinybench, add upstream stable-hash |
| README.md | Add Cross-realm section, update bundle size and benchmarks |
| .size-limit.json | Increase limit from 540B to 600B |
| .changeset/four-beds-visit.md | Bump version and describe crossRealm refactor |
Comments suppressed due to low confidence (2)
tests/unit.spec.ts:382
- [nitpick] Consider making the test more explicit by passing
trueon both sides—e.g.,expect(hash(obj1, true)).toEqual(hash(obj2, true))—so that readers know both hashes use cross-realm mode.
expect(hash(obj1)).toEqual(hash(obj2, true))
src/index.ts:34
- [nitpick] Using a positional boolean flag can be unclear when reading calls. Consider switching to an options object (e.g.,
stableHash(value, { crossRealm: true })) for better readability and future extensibility.
export function stableHash(arg: unknown, crossRealm?: boolean): string {
793699d to
a70f157
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed a70f157 in 1 minute and 18 seconds. Click for details.
- Reviewed
320lines of code in8files - Skipped
1files when reviewing. - Skipped posting
12draft 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. .changeset/four-beds-visit.md:1
- Draft comment:
Changelog entry clearly documents the new crossRealm option. The message is concise and informative. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. .size-limit.json:4
- Draft comment:
Ensure the updated size limit (600B) reflects the actual bundle size output. This change appears consistent with the README update. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. README.md:16
- Draft comment:
The library size update and new cross-realm docs are clear. It might be helpful to mention that when dealing with cross-realm objects, using the crossRealm flag consistently (on all hash calls) may yield more predictable results. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. README.md:159
- Draft comment:
The new Cross-realm example appropriately demonstrates using runInNewContext and passing 'true' for crossRealm. For clarity, consider recommending that both calls use the same crossRealm option when hashing similar objects. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. benchmark.js:9
- Draft comment:
Migration from mitata to tinybench looks smooth. Consider adding a benchmark case for the crossRealm option to evaluate its performance impact separately. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. package.json:73
- Draft comment:
Removal of the 'mitata' dependency and addition of 'tinybench' and 'stable-hash' in devDependencies is noted. Ensure these changes are consistent with the new benchmarking approach. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
7. src/index.ts:34
- Draft comment:
Introducing the optional crossRealm parameter is implemented cleanly. The conditional checks switching between constructor equality and isType are a neat solution for cross-realm object handling. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
8. src/index.ts:57
- Draft comment:
Using Array.isArray when crossRealm is true is appropriate since Array.isArray works reliably across realms. This ensures arrays from different contexts are detected correctly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
9. src/index.ts:66
- Draft comment:
The plain object branch correctly uses isPlainObject in crossRealm mode and a constructor check otherwise. This approach maintains performance while accommodating cross-realm differences. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
10. tests/unit.spec.ts:382
- Draft comment:
Test for cross-realm hashing correctly passes 'true' for the second hash call. This validates the new option as expected. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
11. README.md:54
- Draft comment:
Typographical suggestion: In the comment on this line, consider adding an article ('a') so that it reads "if you're running in a cross-realm environment, it's disabled by default for performance" for improved clarity. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and suggests a typographical change for clarity. It doesn't address any potential issues or improvements in the code itself, nor does it suggest a specific code change or ask for tests. Therefore, it violates the rule against making purely informative comments.
12. benchmark.js:50
- Draft comment:
It looks like there might be a typographical error on line 50. The phrase "Ensure the input is flatten" could be improved. Perhaps it should be "Ensure the input is flattened"? - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_i3g9DGSE3OWfxzoR
You can customize
JounQin
force-pushed
the
refactor/cross-realm
branch
from
a70f157 to
46ff91a
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 46ff91a in 1 minute and 14 seconds. Click for details.
- Reviewed
320lines of code in8files - Skipped
1files when reviewing. - Skipped posting
11draft 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. .changeset/four-beds-visit.md:1
- Draft comment:
Changeset file looks good; the commit message properly reflects the new crossRealm option. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. .size-limit.json:4
- Draft comment:
Updated size limit from 540B to 600B; ensure this increase is expected due to the additional crossRealm logic. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. README.md:159
- Draft comment:
Good addition of the cross-realm example. Consider adding a clarifying note that the second parameter enables cross-realm mode, which may impact performance. - Reason this comment was not posted:
Confidence changes required:10%<= threshold50%None
4. benchmark.js:111
- Draft comment:
Switching benchmarks to tinybench provides clearer output. You might consider adding a benchmark scenario for the crossRealm enabled mode if performance impact is significant. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
5. package.json:86
- Draft comment:
Dependencies have been updated appropriately. Verify that inclusion of 'stable-hash' and 'tinybench' is intentional for benchmarking and internal usage. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
6. src/index.ts:36
- Draft comment:
The new crossRealm parameter is introduced here. Consider adding inline documentation to explain that when crossRealm is true, type checks (e.g. for Date and RegExp) use isType instead of constructor equality. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
7. src/index.ts:57
- Draft comment:
Using Array.isArray when crossRealm is true (vs. constructor === Array otherwise) creates a clear differentiation. Consider documenting this choice to clarify expected behavior across different realms. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
8. src/index.ts:66
- Draft comment:
The plain object check differs based on crossRealm: using isPlainObject vs. constructor check. Ensure this difference is clearly documented so users know what to expect when crossRealm is enabled. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
9. tests/unit.spec.ts:382
- Draft comment:
The cross-realm test now explicitly passes 'true' as the second parameter. This is correct; consider adding additional tests for nested cross-realm objects if needed. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
10. README.md:35
- Draft comment:
Typographical Review: The list items on lines 35–37 have inconsistent spacing before the dash. While lines 31–34 and 38 use two spaces before the '-', lines 35, 36, and 37 omit it. Please adjust the spacing for consistency (e.g., use two spaces before the '-') across all list items. - Reason this comment was not posted:
Comment was on unchanged code.
11. benchmark.js:50
- Draft comment:
Typo: Consider changing 'flatten' to 'flattened' for correct grammatical structure. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_SzmhCTvODn9EN6u3
You can customize
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
16-16: Optional style improvement for redundancy.Consider removing "Originally" for more concise wording.
-A tiny and fast (600B <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 (600B <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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
.changeset/four-beds-visit.md(1 hunks).size-limit.json(1 hunks)README.md(4 hunks)benchmark.js(3 hunks)benchmark.txt(1 hunks)package.json(2 hunks)src/index.ts(3 hunks)tests/unit.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .size-limit.json
- .changeset/four-beds-visit.md
- package.json
- tests/unit.spec.ts
- benchmark.js
🧰 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] 36-36: Do not shadow the global "constructor" 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 (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 18 on macos-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 24 on windows-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
🔇 Additional comments (4)
src/index.ts (1)
34-34: Excellent implementation of cross-realm support.The implementation correctly addresses the past review concerns:
- ✅ Recursive calls now properly propagate the
crossRealmflag- ✅ Type detection uses appropriate methods for cross-realm scenarios
- ✅ Plain objects are handled consistently with
isPlainObjecthelperThe logic is sound and maintains backward compatibility while enabling cross-realm hashing when needed.
Also applies to: 57-57, 62-62, 65-70, 79-79
README.md (2)
16-16: Excellent documentation of the new cross-realm feature.The documentation clearly explains:
- Updated bundle size reflecting the new functionality
- Practical cross-realm usage example with Node.js
vmmodule- Performance consideration (disabled by default)
- Clear API documentation
The example effectively demonstrates when and how to use the cross-realm option.
Also applies to: 34-34, 54-54, 159-179
184-191: Well-formatted benchmark results.The new table format provides clear, comprehensive performance metrics that are easy to compare across different hashing implementations.
benchmark.txt (1)
1-8: Comprehensive benchmark results demonstrate excellent performance.The new table format provides clear performance comparisons showing that
stable-hash-xmaintains competitive performance (7877.4ns avg latency, 138708 ops/s throughput) compared to other hashing implementations. The addition of thecrossRealmfeature doesn't significantly impact performance.
Important
Added
crossRealmoption tostableHashfor consistent cross-realm hashing and updated documentation, tests, and benchmarks.crossRealmoption tostableHashinsrc/index.tsfor consistent hashing across JavaScript realms.README.mdwithcrossRealmusage examples and benchmark results.benchmark.jsto usetinybenchfor performance comparisons.crossRealmoption intests/unit.spec.ts..size-limit.jsonfrom540Bto600B.package.jsonto support new benchmarking tools.This description was created by
for 46ff91a. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Documentation
Refactor
Tests
Chores