fix(delegate): avoid merging into object of Decimal, Date, etc. #1489
Conversation
WalkthroughWalkthroughThe changes introduce the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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
Outside diff range and nitpick comments (1)
packages/runtime/src/enhancements/delegate.ts (1)
Line range hint
309-309: Optimize performance by avoiding the delete operator.The use of the delete operator can impact performance. Consider refactoring to set properties to undefined instead, if applicable.
- delete where[field]; + where[field] = undefined;Also applies to: 332-332, 709-709, 772-772, 780-780
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (2)
packages/runtime/package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!pnpm-lock.yaml,!**/*.yaml
Files selected for processing (2)
- packages/runtime/src/enhancements/delegate.ts (2 hunks)
- tests/regression/tests/issue-1487.test.ts (1 hunks)
Additional context used
Biome
packages/runtime/src/enhancements/delegate.ts
[error] 309-309: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 332-332: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 709-709: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 772-772: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 780-780: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
Additional comments not posted (4)
tests/regression/tests/issue-1487.test.ts (2)
1-2: Ensure correct imports for Decimal handling.The imports for
@zenstackhq/testtoolsanddecimal.jsare correctly used for setting up the database and handling Decimal values respectively.
4-70: Review the test case logic for handling Decimal values.The test case is well-structured and covers the necessary scenarios to ensure that Decimal values are handled correctly. The use of
Decimal.isDecimaland the checks for the correct string representation of Decimal values are appropriate.packages/runtime/src/enhancements/delegate.ts (2)
4-4: Ensure correct import for object type checks.The import of
isPlainObjectfromis-plain-objectis correctly used to enhance the merging behavior of objects, avoiding issues with special types like Decimal and Date.
1098-1098: Review the use ofisMergeableObjectin deep merging.The use of
isPlainObjectas theisMergeableObjectoption indeepmergeis a good practice to ensure that instances of special classes like Decimal and Date are not erroneously merged, preserving their integrity.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
packages/runtime/src/enhancements/delegate.ts (1)
Line range hint
309-309: Consider refactoring to avoid using thedeleteoperator.The
deleteoperator can lead to performance issues due to deoptimizations in JavaScript engines. If the properties being deleted are not required later, consider setting them toundefinedinstead or restructuring the code to avoid the need for deletion.Also applies to: 332-332, 709-709, 772-772, 780-780
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/runtime/src/enhancements/delegate.ts (2 hunks)
Additional context used
Biome
packages/runtime/src/enhancements/delegate.ts
[error] 309-309: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 332-332: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 709-709: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 772-772: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
[error] 780-780: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
Additional comments not posted (1)
packages/runtime/src/enhancements/delegate.ts (1)
1098-1098: UpdateisMergeableObjectto handle special types likeDecimalandDate.This change ensures that
deepmergedoes not incorrectly merge objects of types likeDecimalandDate, which could lead to data corruption or unexpected behavior. Good catch on refining the merge conditions to include arrays and plain objects only.
Fixes #1487