Skip to content

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 2, 2021
Merged

fix: don’t attempt to call prop.get() when prop is nullish #25

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/core/View.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ export default class View extends Component {
const representationIds = [];
this.selections.forEach((v) => {
const { prop } = v.getProperties();
const { representationId } = prop.get('representationId');
const representationId = prop?.get('representationId').representationId;
Copy link
Contributor Author

@FezVrasta FezVrasta Aug 2, 2021

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.

Copy link
Collaborator

@jourdain jourdain Aug 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure either.

Copy link
Contributor Author

@FezVrasta FezVrasta Aug 2, 2021

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.

if (representationId) {
representationIds.push(representationId);
}
Expand All @@ -511,6 +511,9 @@ export default class View extends Component {
return this.selections.map((v) => {
const { prop, compositeID, displayPosition } = v.getProperties();

// Return false to mark this item for removal
if (prop == null) return false;
Copy link
Collaborator

@jourdain jourdain Aug 2, 2021

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?

Copy link
Contributor Author

@FezVrasta FezVrasta Aug 2, 2021

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 😬)


return {
worldPosition: Array.from(
this.openglRenderWindow.displayToWorld(
Expand All @@ -525,7 +528,7 @@ export default class View extends Component {
...prop.get('representationId'),
ray,
};
});
}).filter(Boolean);
}
return [];
}
Expand Down