-
Notifications
You must be signed in to change notification settings - Fork 578
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Added catches for primitive/null values passed as "actual" in assertObjectMatch #3468
fix: Added catches for primitive/null values passed as "actual" in assertObjectMatch #3468
Conversation
I know that @luk3skyw4lker looked at this too. Can we get his blessing in case he has something better? |
Hey @JakeAve maybe you should run the |
@luk3skyw4lker I agree. @kt3k I'm on
but my |
@JakeAve I think these formatting changes were caused by your editor settings (do you enable any auto formatter?). Deno doesn't seem introducing these changes recently (and |
072c325
to
f68e2c1
Compare
throw new TypeError( | ||
`Cannot assertObjectMatch ${ | ||
a === null ? null : `type ${typeof a}` | ||
}`, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about returning a
here instead of throwing TypeError
? That makes the case assertObjectMatch(null, { foo: 42 })
throwing more useful AssertionError instead of TypeError.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to that. I feel like assertObjectMatch is supposed to be for higher ordered objects, so I've been wanting to do a typeerror, but I understand you could have a function that returns either null/undefined or an object. The problem I have is there are ts warnings whenever you try to pass something that couldn't be made into a weakmap, so if we go that route, I wonder if we update the argument types to accept more values than
actual: Record<PropertyKey, any>,
expected: Record<PropertyKey, unknown>,
I'm pushing the updates so we can see how it looks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like assertObjectMatch is supposed to be for higher ordered objects, so I've been wanting to do a typeerror
Ah, right. The previous version might make more sense. Thanks for the clarification of the reasoning.
bb0a84a
to
9a3ca1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Having issues when null and undefined are passed in the 'actual' argument for
assertObjectMatch
. Typeof null is 'object', so in recursion null kept being sent as a weakmap, but should have been assigned asfiltered[key] = value
.Primitive types also could be passed in as the 'actual' value in JS, and they gave a cryptic weakmap error, but it's just a typeerror.
I slightly disagree with the AssertionError instead of the TypeError in issue #3451, but I'm open to suggestions.