FED-1886 Null-safe props: runtime validation of required props

[sydneyjodon-wk]

Motivation

FED-1717 completes support for null-safe props, and this ticket will implement runtime validation that all required late props are specified.

Changes

• Implement runtime validation in UiProps.build that all non-nullable props are specified
  • Use the "more static way" method documented in the prop null safety RFD

    mixin FooProps on UiProps {
      late String requiredProp1;
      String? optionalProp;
      late String requiredProp2;
    }
    mixin $FooProps on FooProps {
      @override
      @mustCallSuper
      void validateRequiredProps() {
        super.validateRequiredProps();
        if (!props.containsKey('FooProps.requiredProp1')) throw RequiredPropError(/* ... */);
        if (!props.containsKey('FooProps.requiredProp2')) throw RequiredPropError(/* ... */);
      }
    }

  • Do not include props considered required by annotation (@requiredProp). It's too disruptive/risky of a changeto make this start to throw; and we'll be deprecating this prop once late is available.
  • Determine when not to run this. assert vs inDevMode.
• Add escape hatch for disabling this validation, e.g., to support cases where required props are cloned onto an element.
  • Perhaps something like UiProps.disableRequiredPropValidation(), which unsets a flag? Or maybe we should have people invoke the component differently, or use the componentFactory directly?
• Add the ability to disable required prop validation on a certain prop

mixin QuxProps on UiProps {
  // Never validate that this prop is set since we know it will always get cloned on
  @disableRequiredPropValidation
  late String qux;
}

• Add tests

Release Notes

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review:

QA Checklist

• Tests were updated and provide good coverage of the changeset and other affected code
• Manual testing was performed if needed
  • Steps from PR author:
  • Anything falling under manual testing criteria outlined in CONTRIBUTING.md

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

• A Frontend Frameworks Design member has reviewed these changes
• There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
• For release PRs - Version metadata in Rosie comment is correct

[aviary3-wk]

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]

Made a partial pass and got through most of the lib changes; one area of feedback but otherwise this looks great! Will make another pass at the tests in a bit

[greglittlefield-wf]

Couple more comments; sorry it took me so long to circle back to the tests!

[greglittlefield-wf]

I think we want to verify more specifically that these throw when the props are invoked, before they're even attempted to be rendered.

So the setup should be more like:

          expect(() {
            (ComponentTest()
              ..requiredNullable = true
            )();
          },
              throwsA(isA<MissingRequiredPropsError>()...

Though I do think that also having test coverage that they render properly, like you have now, is probably a good idea.

[greglittlefield-wf]

Can we also verify that validation is disabled in dart2js? Doesn't have to be exhaustive will all the cases; just one test that missing required props don't cause failures should be sufficient.

[greglittlefield-wf]

Oh interesting, this is so that generated code can use @mustCallSuper, right? Could we add a comment here to that effect.

Kind of gross that it has to be exported from over_react and added to its public API, though I don't think it would cause any issues...

And I can't think of any better alternatives, unless we created an alias for it that was named something else, and maybe deprecated (e.g., @Deprecated('For generated use only) const $overReact$mustCallSuper = mustCallSuper;)... Not sure if that's worth the trouble though.

[greglittlefield-wf]

Sorry, I know you just ripped out the tests for the old boilerplate, but would you mind adding some tests that verify that late props aren't validated upon UiProps invocation for those? That way we're guarded against any regressions to that behavior in the future.

[greglittlefield-wf]

+10