FED-1717 Finalize null-safe props/state implementation and utilities, add tests #869

greglittlefield-wf · 2023-12-13T17:53:07Z

Motivation

The WIP branch had a mostly-complete implementation of null-safe props and state, but it needed to be finished up and tested.

We also needed to implement utilities for safely reading required non-nullable props from partial props objects.

Changes

Finalize generation of late props
- The existing implementation was actually pretty complete, and just needed a bugfix for props that were both late and had an @Accessor annotation
Add utility extensions methods - ui_props_self_typed_extension.dart
- getRequiredProp and getRequiredPropOrNull, for safely reading required non-nullable props when they might not be specified
- getPropKey and containsProp, required for implementing getRequiredProp
- Add basic example in example/builder/partial.dart
Generate $getPropKey (used by getPropKey, needed for utilities above) for all props classes
Add manual implementations of $getPropKey to non-generated classes, with a special case for UiPropsMapView (see breaking changes note)
Don't add documentation in main README and other places; that'll be handled in a follow-up ticket FED-1978
Add tests for
- Props/state of various requiredness and nullabilities, for all boilerplate versions - null_safe_accessor_integration_test.dart in various folders under test/over_react/component_declaration/builder_integration_tests/
  - Reading/writing
  - Requiredness in PropsMeta
- Builder warnings from mixing late and legacy required prop annotations
- New extension methods, including cases reading required props - ui_props_self_typed_extension_test.dart
- $getPropKey:
  - generated implementation for each boilerplate - under test/over_react/component_declaration/builder_integration_tests/
- manual implementations for all non-generated UiProps classes in lib: UiPropsMapView, DomProps, SvgProps, GenericUiProps, ProviderProps, ConsumerProps

Breaking changes

UiPropsMapView (which has been deprecated for a while now) is now abstract and requires subclasses to override selfFactory, in order for it to be able to implement $getPropKey
- Other props classes, including the new props map view boilerplate, generate this, and we can't implement this since we can't call the constructor of consumers' subclasses
ProviderProps.props widened from JsMap to Map - this seemed to be an oversight in the implementation, and this typing prevented it from implementing $getPropKey. I wouldn't expect this to impact consumers.

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

…pKey

…opKey on it

…BoundaryPropsMapView

…red-props

…d-props

aviary-wf · 2023-12-13T17:55:25Z

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 · 2023-12-13T19:01:32Z

example/boilerplate_versions/dart2_only/basic_component1.over_react.g.dart

There are a lot of generated file diffs in this PR with just the $getPropKey changes.

If you'd like to hide them when reviewing this PR to cut down on noise:

Scroll down to the bottom of the /files page to trigger all files to load

Run the following snippet in the console to hide files ending in .over_react.g.dart:
document.querySelectorAll('[data-tagsearch-path$=".over_react.g.dart"]') .forEach((e) => e.hidden = true)

…sses in lib

greglittlefield-wf · 2023-12-13T23:41:55Z

...tion/builder_integration_tests/backwards_compatible/null_safe_accessor_integration_test.dart

All of these null_safe_accessor_integration_test.dart tests should be the same, except for the component boilerplate.

greglittlefield-wf · 2023-12-13T23:43:21Z

lib/src/util/react_util.dart

-        component_base.UiProps {
+        UiProps {


Implementing the generated UiProps makes it so that we can use @override in the methods we're overriding below; not sure why we weren't always doing this.

…d-props

aaronlademann-wf

Just a couple of questions.

Way to put a bow on this Greg!!!

lib/src/util/context.dart

...component_declaration/builder_integration_tests/new_boilerplate/function_component_test.dart

aaronlademann-wf · 2023-12-18T19:53:04Z

test/vm_tests/builder/codegen_test.dart

+
+        test('the late keyword and the requiredProp annotation', () {
+          var body = '''@requiredProp
+              late var bar;''';


I'm imagining this trendy new club in old town... called the var bar

Its open late.

That is a pretty great name 😂

Its open late.

Lolol 😂

aaronlademann-wf

+1

aaronlademann-wf

QA +1

Passing CI

greglittlefield-wf added 28 commits December 1, 2023 11:30

Clean up prop_key_util

21fd0e7

Add utilities for safely accessing props, add/generate UiProps.getPro…

58d9af0

…pKey

Remove ErrorBoundaryPropsMapView so we don't have to implement $getPr…

a563467

…opKey on it

Add example

cd9a865

Clean up whitespace and extra annotation

95a9d1f

Clean up and combine extensions, add doc comments

c5d00a4

Update generated code

84d37aa

Update changelog with removed ErrorBoundaryPropsMapView

11fe1e5

UiPropsMapView: fix inheritance, implement $getPropKey, restore Error…

d66b0f0

…BoundaryPropsMapView

Fix copypasta'd comment

1eca35a

Update generated code

42266a6

Revert ErrorBoundaryPropsMapView removal workaround

a36b6ae

Add integration test for null safe props

91e5627

Update gold files with $getPropKey changes

0840751

Add tests for generated getPropKey implementation

481269f

Clean up comments, ignore $getPropKey usage

712af2d

Add null-safe accessor tests for all boilerplate versions

ef4382c

Add tests for null-safe state

6f7409c

Add nullable state tests to other boilerplate versions

8347f58

Merge branch 'gold-file-workflow-improvements' into null-safety-requi…

523147b

…red-props

Update gold files

992c32e

Add tests for getPropKey, getRequiredProp

a1ef6f4

Add tests for getRequiredPropOrNull

ecd3193

Add tests for error rethrowing

f8f967c

Fix Accessor annotation not working with late

e24c106

Add tests for late props: accessor, prop metadata

a33a18c

Merge remote-tracking branch 'origin/v5_wip' into null-safety-require…

b5e3e15

…d-props

Format

fa284b5

rmconsole-wf added the Merge Requirements Unmet label Dec 13, 2023

greglittlefield-wf commented Dec 13, 2023

View reviewed changes

greglittlefield-wf added 3 commits December 13, 2023 16:01

Add $getPropKey implementations to non-generated props classes

33d58f9

Test that getPropKey arg is typed correctly

6b6ce11

Add tests for getPropKey/propKeyNamespace for non-generated props cla…

17c40bd

…sses in lib

greglittlefield-wf commented Dec 13, 2023

View reviewed changes

Add changelog entry for ProviderProps.props

b831e9c

greglittlefield-wf marked this pull request as ready for review December 13, 2023 23:51

greglittlefield-wf requested review from aaronlademann-wf and sydneyjodon-wk December 13, 2023 23:51

greglittlefield-wf added 4 commits December 18, 2023 11:16

Merge remote-tracking branch 'origin/v5_wip' into null-safety-require…

38afaf3

…d-props

Update generated files

cb08559

Update gold files

0ee1e05

Add getPropKey test cases to newly-copied non-null-safe test files

4e6386d

aaronlademann-wf reviewed Dec 18, 2023

View reviewed changes

Upgrade staticMeta comments to TODOs with ticket

80a4d67

aaronlademann-wf approved these changes Dec 18, 2023

View reviewed changes

aaronlademann-wf approved these changes Jan 5, 2024

View reviewed changes

greglittlefield-wf merged commit 31940a6 into v5_wip Jan 5, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FED-1717 Finalize null-safe props/state implementation and utilities, add tests #869

FED-1717 Finalize null-safe props/state implementation and utilities, add tests #869

greglittlefield-wf commented Dec 13, 2023 •

edited

Loading

aviary-wf commented Dec 13, 2023

greglittlefield-wf Dec 13, 2023

greglittlefield-wf Dec 13, 2023

greglittlefield-wf Dec 13, 2023

aaronlademann-wf left a comment

aaronlademann-wf Dec 18, 2023

aaronlademann-wf Dec 18, 2023

greglittlefield-wf Dec 18, 2023

aaronlademann-wf left a comment

aaronlademann-wf left a comment

FED-1717 Finalize null-safe props/state implementation and utilities, add tests #869

FED-1717 Finalize null-safe props/state implementation and utilities, add tests #869

Conversation

greglittlefield-wf commented Dec 13, 2023 • edited Loading

Motivation

Changes

Breaking changes

Release Notes

Review

QA Checklist

Merge Checklist

aviary-wf commented Dec 13, 2023

Security Insights

Action Items

greglittlefield-wf Dec 13, 2023

Choose a reason for hiding this comment

greglittlefield-wf Dec 13, 2023

Choose a reason for hiding this comment

greglittlefield-wf Dec 13, 2023

Choose a reason for hiding this comment

aaronlademann-wf left a comment

Choose a reason for hiding this comment

aaronlademann-wf Dec 18, 2023

Choose a reason for hiding this comment

aaronlademann-wf Dec 18, 2023

Choose a reason for hiding this comment

greglittlefield-wf Dec 18, 2023

Choose a reason for hiding this comment

aaronlademann-wf left a comment

Choose a reason for hiding this comment

aaronlademann-wf left a comment

Choose a reason for hiding this comment

greglittlefield-wf commented Dec 13, 2023 •

edited

Loading