-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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(): Replace 'hasOwn' with 'in' operator in typeAssertions check #9812
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
Yes hasOwn is definitely out of our supported browsers specs. |
@@ -46,4 +44,4 @@ export const isPath = (fabricObject?: FabricObject): fabricObject is Path => { | |||
export const isActiveSelection = ( | |||
fabricObject?: FabricObject | |||
): fabricObject is ActiveSelection => | |||
!!fabricObject && Object.hasOwn(fabricObject, 'multiSelectionStacking'); | |||
!!fabricObject && 'multiSelectionStacking' in fabricObject; |
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.
keep in mind that in
also checks if property exists down the prototype chain, where hasOwnProperty
just makes sure that it is a direct property of an object
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.
Yes i was commenting while you posted this, and indeed it also make more sense for the subclasses.
While 'multiSelectionStacking' or 'source' are a property that get defined over and over through the getDefaults() in the contructors, that may not be the case if getDefaults() is emptied and you decide to work on the prototype chain with values. In that case on a subclass ownProperty would fail, but in no.
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.
Yeah I used in
as well because for the filler.source
case it should be an own property typically, whereas with the ActiveSelection case it's even safer if the class is subclassed
i'll merge this tomorrow if no additional comments are made |
Object.hasOwn
is a relatively-recent API so it should not be used. Indeed I noticed this by looking at our production exceptions. I've replaced the usage with justin
operator, which for some reason makes TS happier thanobj.hasOwnProperty()
.https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwn