FED-202 Return jsUndefined instead of null when children prop is empty

[sydneyjodon-wk]

Some JS components, like Tab expect no children to be passed to them.

However, when rendered from Dart, Tab's propTypes warn about children not being supported, even when no children are provided:

react.js:6902 Warning: Failed prop type: The prop `children` is not supported. Please remove it.
    at Tab (http://localhost:8080/packages/react_material_ui/react-material-ui-development.umd.js:46607:19)

This is because, when there are no children, ReactJsComponentFactoryProxy passes null into createElement, and the unsupportedProp validation checks warns when the prop is undefined.

Changes

• Return jsUndefined instead of null from generateChildren when the children prop is empty
• Add regression test

QA Instructions

• Verify CI passes
• Verify good test coverage

[aviary-wf]

Security Insights

No security relevant content was detected by automated scans.

Action Items

• Review PR for security impact; comment "security review required" if needed or unsure
• Verify aviary.yaml coverage of security relevant code

---

Questions or Comments? Reach out on Slack: #support-infosec.

[sydneyjodon-wk]

I don't know if there's a more straightforward way to test that props.children is specifically undefined and not null - open to suggestions

[greglittlefield-wf]

I thought maybe we could change this expectation:

-expect(getChildren(instance), shouldAlwaysBeList ? [] : isNull);
+// We want to make sure this is undefined and not null, so we have to use `same`
+// which uses `identical`, since in Dart null and undefined are otherwise interchangeable.
+expect(getChildren(instance), shouldAlwaysBeList ? [] : same(jsUndefined));

But it looks like that doesn't even work, since the identical check gets compiled to == instead of === , and identical(jsNull, jsUndefined) is true 😕.

So, checking it from the JS seems like the way to go to me!

Related to my comment above, we could also potentially check whether it's undefined on the ReactElement without rendering it, if the rendering/throwing causes issues or complicates things.

window.hasUndefinedChildren = (reactElement) => reactElement.props.children === undefined;

 expect(hasUndefinedChildren(JsNoChildren({})), isTrue);

[sydneyjodon-wk]

Oh that's a lot nicer! Thank you!

[aaronlademann-wf]

+1

[greglittlefield-wf]

I thought maybe we could change this expectation:

-expect(getChildren(instance), shouldAlwaysBeList ? [] : isNull);
+// We want to make sure this is undefined and not null, so we have to use `same`
+// which uses `identical`, since in Dart null and undefined are otherwise interchangeable.
+expect(getChildren(instance), shouldAlwaysBeList ? [] : same(jsUndefined));

But it looks like that doesn't even work, since the identical check gets compiled to == instead of === , and identical(jsNull, jsUndefined) is true 😕.

So, checking it from the JS seems like the way to go to me!

Related to my comment above, we could also potentially check whether it's undefined on the ReactElement without rendering it, if the rendering/throwing causes issues or complicates things.

window.hasUndefinedChildren = (reactElement) => reactElement.props.children === undefined;

 expect(hasUndefinedChildren(JsNoChildren({})), isTrue);

[greglittlefield-wf]

Just to make sure this throws when we'd expect it to and can't result in a false positive, could we add an additional check here?

      expect(() => react_dom.render(JsNoChildren({}, jsNull), Element.div()), throws,
         reason: 'test setup check: throws when children are not undefined');

[greglittlefield-wf]

+10

@Workiva/release-management-p

[rmconsole-wf]

+1 from RM