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-2444 Null safety preparation for conditional prop function calls #285

Merged
merged 12 commits into from
Apr 18, 2024

Conversation

aaronlademann-wf
Copy link
Contributor

@aaronlademann-wf aaronlademann-wf commented Apr 10, 2024

Motivation

Functions passed in as prop values are often null checked before being called using patterns that do not migrate nicely to null-safety - requiring the use of the ! assertion like so:

if (props.onClick != null) {
  props.onClick!(event);
}
// or
if (props.onClick != null) props.onClick!(event);

Changes

  1. Add a suggestor that removes the separate conditional null check and instead adds a null-aware ? operator
  2. Add the suggestor to the existing null_safety_prep executable.
  3. Write tests.

What it does

Before

if (props.onClick != null) {
  props.onClick(event);
}
// or
if (props.onClick != null) props.onClick(event);

After

props.onClick?.call(event);

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

Copy link
Contributor

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

Looking good, left some comments on cases to handle and tests

@aaronlademann-wf
Copy link
Contributor Author

All feedback addressed!

Copy link
Contributor

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

Thanks for addressing that feedback! Just one more small comment, otherwise LGTM; I also tested it locally and it works great!

Copy link
Contributor

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

+10

Copy link
Contributor

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

+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 7063131 into master Apr 18, 2024
6 checks passed
@rmconsole7-wk rmconsole7-wk deleted the FED-2444_null-aware-fn-prop-calls branch April 18, 2024 17:57
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