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
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@ import React from 'react';
import ObjectTree from '@components/ObjectTree';

function FactValue({ className, data }) {
return typeof data === 'object' ? (
<ObjectTree className={className} data={data} />
) : (
<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 😉

return <span className={className}>null</span>;
}
if (typeof data === 'object') {
return <ObjectTree className={className} data={data} />;
}
return <span className={className}>{`${data}`}</span>;
}

export default FactValue;
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ describe('FactValue component', () => {
plain: true,
stringRepresentation: 'true',
},
{
type: 'null',
plain: null,
stringRepresentation: 'null',
},
];

it.each(scalarScenarios)(
Expand Down
7 changes: 6 additions & 1 deletion assets/js/components/ObjectTree/ObjectTree.stories.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,10 @@ export default {
};

export function Normal(args) {
return <ObjectTree data={objectTreeFactory.build()} {...args} />;
return (
<ObjectTree
data={objectTreeFactory.build({ empty_array: [], empty_object: {} })}
{...args}
/>
);
}
7 changes: 5 additions & 2 deletions assets/js/components/ObjectTree/ObjectTree.test.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { flattenTree, treeify } from './tree';

describe('ObjectTree component', () => {
it('should render correctly', () => {
const data = objectTreeFactory.build();
const data = objectTreeFactory.build({ empty_array: [], empty_object: {} });
const {
number,
string,
Expand All @@ -21,6 +21,7 @@ describe('ObjectTree component', () => {

expect(screen.getByText(number)).toBeVisible();
expect(screen.getByText(string)).toBeVisible();
expect(screen.getAllByText('{}')).toHaveLength(2);
expect(screen.queryByText(nestedString)).toBeNull();
expect(screen.queryByText(firstArrayElement)).toBeNull();

Expand All @@ -42,7 +43,7 @@ describe('flattenTree and treeify', () => {

const [
{
children: [firstChildID, _second, _third, fourthChildID],
children: [firstChildID, _second, _third, fourthChildID, fifthChildID],
},
] = children;

Expand All @@ -61,5 +62,7 @@ describe('flattenTree and treeify', () => {

expect(children[firstComplexObjectChild].parent).toBe(fourthChildID);
expect(children[secondComplexObjectChild].parent).toBe(fourthChildID);

expect(children[fifthChildID].value).toBe(null);
});
});
16 changes: 15 additions & 1 deletion assets/js/components/ObjectTree/ObjectTreeNode.jsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,22 @@
import React from 'react';
import classNames from 'classnames';

import { has } from 'lodash';

import ObjectTreeIcon from './ObjectTreeIcon';

const renderElement = (element) => {
if (element.value === null) {
return 'null';
}

if (!has(element, 'value') && element.children.length === 0) {
return '{}';
}

return element.value;
};

function ObjectTreeNode({
element,
isBranch,
Expand All @@ -24,7 +38,7 @@ function ObjectTreeNode({
{isBranch ? (
<ObjectTreeIcon expanded={isExpanded} />
) : (
<span className="ml-2">{element.value}</span>
<span className="ml-2">{renderElement(element)}</span>
)}
</div>
);
Expand Down
4 changes: 4 additions & 0 deletions assets/js/components/ObjectTree/tree.js
Original file line number Diff line number Diff line change
Expand Up @@ -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

return { name: key, value: null };
}

if (dataType === 'object') {
return treeify(key, element);
}
Expand Down
1 change: 1 addition & 0 deletions assets/js/lib/test-utils/factories/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,4 +110,5 @@ export const objectTreeFactory = Factory.define(() => ({
nestedNumber: faker.number.int(),
nestedString: faker.word.noun(),
},
null: null,
}));