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

FED-1910 Fix react_dom API typings, add tests #382

Merged
merged 8 commits into from
Nov 17, 2023

Conversation

greglittlefield-wf
Copy link
Collaborator

@greglittlefield-wf greglittlefield-wf commented Nov 16, 2023

Motivation

The existing nullability/typings for ReactDom.findDomNode and ReactDom.render exported from package:react/react_client/react_interop.dart are incorrect.

class ReactDom {
  Element findDOMNode(dynamic object) {/*...*/}
  ReactComponent render(ReactElement component, Element element) {/*...*/}
  /*...*/
}

findDOMNode returns null in many cases, but its return type is incorrectly non-nullable.

render returns null for some cases (function components, null), Element for DOM components, and CharacterData for strings and numbers, but is incorrectly typed as non-nullable ReactComponent. The component argument also accepts null and other "ReactNode" arguments to rendered, but its type is incorrectly non-nullable and restricted to just ReactElement.

These bad typings cause runtime errors in some cases. Unfortunately, there wasn't good test coverage around these methods.

These typings also only affect these APIs under the ReactDom class in package:react/react_client/react_interop.dart, and not the top-level Function-typed findDOMNode and render APIs exported from package:react/react_dom.dart, which most consumers use.

Because these typings were incorrect and will lead to runtime errors in some cases, and the changes have a low likelihood of causing breakages, it feels appropriate to release these changes as a patch.

Solution

  • Fix bad typings:
    -Element findDOMNode(dynamic object);
    +Element? findDOMNode(dynamic object);
    
    -ReactComponent render(ReactElement component, Element element);
    +dynamic render(dynamic component, Element element);
  • Add tests
  • Update changelog
  • Fix cast_nullable_to_non_nullable warnings failing CI, which appear to be unrelated to this PR

@aviary3-wk
Copy link

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.

@greglittlefield-wf greglittlefield-wf changed the title Fix react_dom entrypoint typings, add tests Fix react_dom API typings, add tests Nov 17, 2023
@greglittlefield-wf greglittlefield-wf marked this pull request as ready for review November 17, 2023 00:38
@rmconsole6-wk rmconsole6-wk changed the title Fix react_dom API typings, add tests FED-1910 Fix react_dom API typings, add tests Nov 17, 2023
Copy link
Collaborator

@aaronlademann-wf aaronlademann-wf left a comment

Choose a reason for hiding this comment

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

+1

Love the new test coverage!

@greglittlefield-wf
Copy link
Collaborator Author

@Workiva/release-management-p

Copy link

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

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

+1 from RM

@rmconsole5-wk rmconsole5-wk merged commit c59ba12 into master Nov 17, 2023
6 checks passed
@rmconsole5-wk rmconsole5-wk deleted the fix-react-dom-typings branch November 17, 2023 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants