-
Notifications
You must be signed in to change notification settings - Fork 24
fix: don’t attempt to call prop.get() when prop is nullish #25
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
Conversation
| const { prop, compositeID, displayPosition } = v.getProperties(); | ||
|
|
||
| // Return false to mark this item for removal | ||
| if (prop == null) return false; |
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.
Should we write if (!prop) { return false; } instead?
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 used to == null because it's the quickest way to check for null and undefined while not checking for any other falsy value, I know in this case it will always be either an object-type or null/undefined so it's not strictly needed, i't just clearer to me. (I come from a strict Flow-type checking background, sorry 😬)
| this.selections.forEach((v) => { | ||
| const { prop } = v.getProperties(); | ||
| const { representationId } = prop.get('representationId'); | ||
| const representationId = prop?.get('representationId').representationId; |
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 not sure if your build pipeline supports the optional chaining operator, I can replace it with something else if not.
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.
Not sure either.
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.
It is, I just checked the dist output.
|
Thanks, does that solve your issue? Can you rename your commit like |
Yes it works indeed |
|
Thanks for your contribution! |
|
🎉 This PR is included in version 1.5.10 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fixes #24