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-2298 Add ReactNode typedef #384

Merged
merged 8 commits into from
Mar 4, 2024
Merged

Conversation

aaronlademann-wf
Copy link
Collaborator

Motivation

In React, a "React node" is a value that can be returned from a component's render or used as children, and contains more values than just ReactElement.

TypeScript captures this type as a ReactNode type

Now that Dart has type aliases (in SDK 2.13 and up), can name this type so that consumers can be more expressive in how they type their code (and also to help them avoid using ReactElement, which in some cases is overly restrictive).

Changes

  • Add ReactNode typedef, which is an alias for Object?.
  • Use this new typedef where appropriate

@aviary-wf
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.

Comment on lines 33 to 35
final ForwardRefTestComponent = forwardRef2((props, ref) {
return null;
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another nice thing about switching from a dynamic return type to Object? is that for function components, you get an analysis warning if you forget to return something.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This actually seems like a breaking change, so unfortunately I think we'll have to change the return type of JsFunctionComponent/JsForwardRefFunctionComponent and Component(2).render back to dynamic

@aaronlademann-wf aaronlademann-wf force-pushed the FED-2298_ReactNode_typedef branch from a7a77f5 to ead4b8e Compare March 1, 2024 21:23
Copy link
Collaborator

@greglittlefield-wf greglittlefield-wf left a comment

Choose a reason for hiding this comment

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

A couple comments about breakages, otherwise LGTM!

lib/react_client/react_interop.dart Outdated Show resolved Hide resolved
Comment on lines 33 to 35
final ForwardRefTestComponent = forwardRef2((props, ref) {
return null;
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

This actually seems like a breaking change, so unfortunately I think we'll have to change the return type of JsFunctionComponent/JsForwardRefFunctionComponent and Component(2).render back to dynamic

lib/react_client/react_interop.dart Outdated Show resolved Hide resolved
Copy link
Collaborator

@greglittlefield-wf greglittlefield-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, QAing now...

Copy link
Collaborator

@greglittlefield-wf greglittlefield-wf left a comment

Choose a reason for hiding this comment

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

  • Changes look good
  • Code analyzes and builds without error in over_react and other private consumers

+10

@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

@rmconsole7-wk rmconsole7-wk merged commit 583cf18 into master Mar 4, 2024
6 checks passed
@rmconsole7-wk rmconsole7-wk deleted the FED-2298_ReactNode_typedef branch March 4, 2024 18:53
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.

5 participants