Skip to content
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

Handle fact value with null value #1878

Merged
merged 3 commits into from
Oct 2, 2023
Merged

Handle fact value with null value #1878

merged 3 commits into from
Oct 2, 2023

Conversation

arbulu89
Copy link
Contributor

@arbulu89 arbulu89 commented Sep 27, 2023

Description

Handle null value in the ObjectTree. Without this fix the Object.keys(data) above panics.
Related to: trento-project/wanda#297

image

PD: As the objectree treats arrays and objects as the same, I think having the {} is good enough.

How was this tested?

UT added

@arbulu89 arbulu89 added the bug Something isn't working label Sep 27, 2023
@@ -46,6 +46,10 @@ export const treeify = (name, data) => ({
const { [key]: element } = data;
const dataType = typeof element;

if (element == null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the piece of code which fixes the actual error

@arbulu89 arbulu89 force-pushed the object-tree-render-null branch from 5f68c24 to cc67448 Compare September 27, 2023 09:31
@arbulu89 arbulu89 marked this pull request as ready for review September 27, 2023 11:29
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

LGTM. Just a tiny comment.

) : (
<span className={className}>{`${data}`}</span>
);
if (data === null) {
Copy link
Member

Choose a reason for hiding this comment

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

Just curious, this is needed because typeof null is object, right? 😄

Besides this I am wondering whether it would make sense having this check in ObjectTree itself. wdyt?

Copy link
Contributor Author

@arbulu89 arbulu89 Sep 27, 2023

Choose a reason for hiding this comment

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

Just curious, this is needed because typeof null is object, right? 😄

Yes, it is a bug in js that it won't be fixed in 10000 years.

Besides this I am wondering whether it would make sense having this check in ObjectTree itself. wdyt?

We have the same fix in the ObjectTree. We need to have in both places because we can get a plain fact value, so we don't always use ObjectTree

Copy link
Member

Choose a reason for hiding this comment

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

fine with that 😉

Copy link
Member

@EMaksy EMaksy left a comment

Choose a reason for hiding this comment

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

Hey man thanks codewise LGTM!

@arbulu89 arbulu89 merged commit c96bc4e into main Oct 2, 2023
@arbulu89 arbulu89 deleted the object-tree-render-null branch October 2, 2023 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

3 participants