fix(utilities): ensure consistent typing for objectEntries, objectKeys and objectValues#857
Merged
favna merged 6 commits intosapphiredev:mainfrom Jan 29, 2025
GeniusTimo:patch-1
Merged
fix(utilities): ensure consistent typing for objectEntries, objectKeys and objectValues#857favna merged 6 commits intosapphiredev:mainfrom GeniusTimo:patch-1
objectEntries, objectKeys and objectValues#857favna merged 6 commits intosapphiredev:mainfrom
GeniusTimo:patch-1
Conversation
objectEntries, objectKeys and objectValues functions to make them more consistentobjectEntries, objectKeys and objectValues
kyranet
previously approved these changes
Jan 19, 2025
favna
requested changes
Jan 19, 2025
Member
favna
left a comment
There was a problem hiding this comment.
Could you add unit tests for this using vitest testing types? To ensure that the same mistake is not made in the future because these types are quite complex.
Contributor
Author
|
I'm not familiar with vitest yet, but I've tried my best. Since we can't test the type assertion within the function (at least to my knowledge), I feel like checking the return type for arrays is all we can add? |
favna
approved these changes
Jan 29, 2025
objectEntries, objectKeys and objectValuesobjectEntries, objectKeys and objectValues
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I noticed that these functions have return types that do not match the type assertions within their implementations. After reviewing the pull requests that introduced these functions, I believe the idea to use
ArrayLike<infer Values>(see #474 (comment)) may have unintentionally caused some confusion. The idea for theobjectValuesfunction was only implemented to the type assertion within the function and not yet for its return type (de87080 ✔)To improve consistency, the same idea was adapted for the
objectEntriesfunction (see #471 (comment)). However, it was implemented as ifObject.entriesreturned the values, not the entries, when dealing with array-like objects (9d6d3e8 ✔)Because the return type of the function was not updated to reflect this change, it had no real impact. To align with the intended consistency, I have updated the return type to match the corrected type assertion (34c3205 ✔)
The type of the values within an array-like object is not relevant to the
objectKeysfunction. However, the idea was also applied to that type assertion (see #472 (comment)) Because the return type was not adjusted accordingly, this change also had no actual effect. I have fixed this as well (b43fba0 ✔)This pull request is meant to reduce potential confusion for future contributors. The details regarding the previous implementation are provided for traceability only and are not intended as criticism of or offense to anyone involved back then.