🐛 Fix isObjectOf for non pure object predication
#61
Conversation
Prior to v1.16.0, `isObjectOf` would return `true` for an instance.
`isObjectOf` should support non pure object (e.g. `Date` instance) but `isRecord` check if the value is a pure object or not.
|
Warning Rate Limit Exceeded@lambdalisue has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 26 minutes and 11 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe update involves refining the Changes
Poem
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #61 +/- ##
==========================================
- Coverage 99.37% 99.36% -0.02%
==========================================
Files 9 9
Lines 638 627 -11
Branches 68 66 -2
==========================================
- Hits 634 623 -11
Misses 4 4 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- is/factory.ts (1 hunks)
- is/factory_test.ts (2 hunks)
Additional comments: 3
is/factory.ts (1)
- 511-511: The check
if (x == null || typeof x !== "object") return false;correctly ensures thatxis neithernullnor a non-object type. This is a crucial step for the function's logic to predicate objects accurately.is/factory_test.ts (2)
- 15-15: The addition of
isFunctionto the imports aligns with the PR's objective to enhance type checking capabilities, specifically for handling non-pure objects likeDateinstances. This change is necessary for the new test case introduced forisObjectOf.- 622-628: The new test case for
isObjectOfeffectively validates the function's ability to predicate non-pure objects, specifically using aDateinstance and checking for the presence of thegetFullYearmethod. This test directly addresses the PR's objective to fix theisObjectOffunction for non-pure object predication and ensures that the functionality works as expected.
| // Check each values | ||
| for (const k in predObj) { | ||
| if (!predObj[k](x[k])) return false; | ||
| if (!predObj[k]((x as T)[k])) return false; |
There was a problem hiding this comment.
The loop for (const k in predObj) iterates over the keys in predObj and uses the predicates to check the corresponding properties in x. However, there's a potential issue with properties that are optional in predObj. If predObj specifies a property as optional using is.OptionalOf(), and that property is not present in x, this check will incorrectly return false.
To address this, you should verify whether each property is optional and, if so, only perform the check if the property exists in x.
- if (!predObj[k]((x as T)[k])) return false;
+ const predicate = predObj[k];
+ if (isWithOptional(predicate) && !(k in x)) continue;
+ if (!predicate((x as T)[k])) return false;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if (!predObj[k]((x as T)[k])) return false; | |
| const predicate = predObj[k]; | |
| if (isWithOptional(predicate) && !(k in x)) continue; | |
| if (!predicate((x as T)[k])) return false; |
There was a problem hiding this comment.
No. In that case, predObj[k] return a predicate function that return true on undefined.
There was a problem hiding this comment.
@lambdalisue: Understood, thank you for clarifying how the predicate functions are designed to handle undefined values for optional properties. This approach elegantly ensures that the implementation correctly supports optional properties without needing additional checks.
Some instance attributes are non enumerable, so we should not check.
lambdalisue
force-pushed
the
fix-isobjectof
branch
from
fe43f2b to
14a83f7
Compare
Prior to v1.6.0,
isObjectOfsupported predicating non pure object likeDateinstance.This PR add tests for this case and fix the behavior.
Summary by CodeRabbit