Skip to content

Conversation

@aaronlademann-wf
Copy link
Contributor

@aaronlademann-wf aaronlademann-wf commented Dec 13, 2023

Motivation

The null safety over_react major release will make a breaking change in which FluxPropsMixin's store and actions props are required, so that they can be made non-nullable.

Making store required shouldn't affect most usages, but actions will definitely affect some consumers.

While the actions prop will be present in usages in most cases, there are usages that only wire up a component to redraw on the store, because the actions aren't used.

Changes

  1. Create a RequiredFluxProps() codemod class that patches invoked and un-invoked builders that mix-in FluxUiPropsMixin when either the actions or store prop(s) are missing.
    • Adds the missing setter with a null value unless an in-scope var with matching type is found.
    • If an in-scope var is found with a matching type - that var is set as the value.
  2. Write tests for all conceivable patch cases

QA

  1. Verify the changes produced by this batch that executes this codemod.

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

I didn’t touch these files.

bool _isFnComponentDeclaration(Expression? varInitializer) =>
varInitializer is MethodInvocation &&
varInitializer.methodName.name.startsWith('uiF');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should account for both uiFunction and uiForwardRef

node.initializer != null &&
node.initializer is SimpleStringLiteral) {
SimpleStringLiteral literal = node.initializer as SimpleStringLiteral;
SimpleStringLiteral literal = node.initializer! as SimpleStringLiteral;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were lints causing CI failures in a few misc. files that are unrelated to the scope of this PR.

@aaronlademann-wf aaronlademann-wf changed the title FED-1919 Add codemod to add required flux actions / store prop(s) FED-1919 Codemod to add required flux actions / store prop(s) Dec 13, 2023
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.

Made a pass at everything except tests; will take a look at those after lunch

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.

Done with second pass, just a couple small comments on tests; overall looks 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.

+1 with some #nits

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.

Couple more comments after reviewing the test batch

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

@greglittlefield-wf
Copy link
Contributor

@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

@rmconsole3-wf rmconsole3-wf merged commit b4dbaad into master Jan 5, 2024
@rmconsole3-wf rmconsole3-wf deleted the batch/fedx/FED-1919_required_flux_actions branch January 5, 2024 21:13
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