FED-1978 Null-safe props documentation #916

greglittlefield-wf · 2024-05-06T18:39:08Z

Motivation

We needed some documentation on required props and how they relate to null safety.

We also need to update code blocks in documentation to use null safety.

Changes

Small README cleanup
- Make null safety the featured migration guide
- Add basic docs around function components (can't believe we didn't have any 😓) #boyscouting
Add required props documentation
Update code blocks with null safety
- In README
- In doc comments

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 · 2024-05-06T18:40:43Z

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 · 2024-05-06T18:41:23Z

doc/null_safety_and_required_props.md

@@ -0,0 +1,411 @@
+# Null safety and required props


GitHub's "rich diff" viewer apparently doesn't render alerts very well, so I'd recommend viewing this file directly: https://github.com/Workiva/over_react/blob/null-safety-docs/doc/null_safety_and_required_props.md

greglittlefield-wf · 2024-05-06T18:41:40Z

README.md

@@ -243,11 +243,17 @@ The use-case for composing multiple props mixins into a single component props c

 #### UiProps as a Map

+> [!WARNING]


GitHub's "rich diff" viewer apparently doesn't render alerts very well, so I'd recommend viewing this file directly: https://github.com/Workiva/over_react/blob/null-safety-docs/README.md#uiprops-as-a-map

greglittlefield-wf · 2024-05-06T18:53:38Z

README.md

-__There have been a lot of fantastic [improvements in this library recently](https://pub.dev/packages/over_react#-changelog-tab-)__, all of which require some action on your part if you have existing components built prior to the `3.1.0` release of OverReact.  __We've done everything we can to make the migrations as painless as possible__ - with the vast majority of changes being handled by some codemod scripts you can run in your libraries locally. As always, if you encounter issues while working through the migration, you can reach out to us in [our gitter chat](https://gitter.im/over_react/Lobby), or [open a new issue][new-issue].
-
-__First, you should upgrade your components to `UiComponent2`__. Check out the [`UiComponent2` Migration Guide](doc/ui_component2_transition.md) to learn about the benefits of `UiComponent2`, the codemod script you can run, and other updates you may need to make manually.
-
-__Once you have migrated your components to `UiComponent2`__, you're ready to start using the "v3" component boilerplate - which is a _massive_ quality of life improvement for component authors! Check out the [Component Boilerplate Migration Guide](doc/new_boilerplate_migration.md) to learn about the benefits of the new boilerplate, the codemod script you can run, and other updates you may need to make manually.


All this info (that's still relevant) is captured in the other migration guides

aaronlademann-wf

This is great! Just found a couple of things...

README.md

aaronlademann-wf · 2024-05-08T16:45:09Z

README.md

+    bool? isDisabled;
+    Iterable<String>? items;


Is there a reason these are nullable here, but not on line 817-818?

In those lines, they're props that are defaulted in a class component's defaultProps, whereas here, they're defaulted in a function component

doc/new_boilerplate_migration.md

lib/src/component/hooks.dart

aaronlademann-wf · 2024-05-08T16:48:43Z

lib/src/component/hooks.dart

 /// UiFactory<FancyInputProps> FancyInput = uiForwardRef(
 ///   (props, ref) {


Since we're using a ref declared in props instead of the forwarded ref, we probably should just make this uiFunction

Suggested change

/// UiFactory<FancyInputProps> FancyInput = uiForwardRef(

/// (props, ref) {

/// UiFactory<FancyInputProps> FancyInput = uiFunction(

/// (props) {

Actually, the ref passed to a component isn't available in props.ref and so I believe you need to use forwardRef to use useImperativeHandle (see React docs here)

doc/null_safety_and_required_props.md

sydneyjodon-wk

These docs are great!! Really clear and helpful! I just had some minor comments

doc/null_safety/null_safe_migration.md

lib/src/component/hooks.dart

doc/null_safety_and_required_props.md

sydneyjodon-wk · 2024-05-08T19:52:09Z

doc/null_safety_and_required_props.md

+OverReact supports providing defaults for optional props in the following cases:
+
+| Nullability  | Class Component | Function component | 
+|--------------|-----------------|--------------------|
+| Non-nullable | Yes<sup>1</sup> | No                 | 
+| Nullable     | Yes             | Yes<sup>2</sup>    |
+
+1. _Props are declared the same way required props are_
+2. _Easiest when `null` is treated the same as the default_


This is a great explanation! I was expecting the following subsections to map to the three Yes boxes, but it seemed kind of unclear - could there be an example here (like maybe the last example - line 343) or links to the different sections or maybe rename the sections so it's really easy to find an example of each of these situations in the table?

I took a stab and making this clearer, and also added a bit more content: d14ca46

sydneyjodon-wk · 2024-05-08T19:54:51Z

doc/null_safety_and_required_props.md

+
+### Defaulting nullable props using `??`
+
+In over_react function components, prop defaulting for nullable props is typically implemented using null-aware `??` operators. As a result, unspecified props and explicit `null` values are treated the same.


Is this also how you default nullable props in class components or is it only a function component thing? maybe the section headers could be updated depending on which is the case

It's a function component thing, but you can also do this in class components. I think I had originally written the header in a way to not exclude class components; I'll try updating this to be clearer

greglittlefield-wf · 2024-05-10T21:42:48Z

Okay @aaronlademann-wf and @sydneyjodon-wk, I think I've addressed all your feedback!

sydneyjodon-wk

+10 Looks great to me!!

aaronlademann-wf

@Workiva/release-management-pp

rmconsole-wf

+1 from RM

greglittlefield-wf added 15 commits May 1, 2024 14:49

Add some null-safety docs

0d782a8

Update some examples with null safety

0ef8d13

Markdown syntax fixes, stop using footnotes

8f42d01

Add docs for unsafe required prop accesses

35d79b0

Fix syntax in lint docs

4a9d991

Add docs around disabling required prop validation

f3164a8

Fix formatting

45fff3a

Move required prop docs into separate file

23815fd

Merge remote-tracking branch 'origin/master' into null-safety-docs

fe302ae

Clean up migration guides section, link to new docs

4ab1f21

Formatting fixes

afe414b

Add reference to prop null safety docs

e2e3c8b

Add bare-bones function component docs to main README

a8b5a1c

Address fixmes

9d354cc

Refine warning callouts

c556f14

rmconsole-wf added the Merge Requirements Unmet label May 6, 2024

greglittlefield-wf commented May 6, 2024

View reviewed changes

greglittlefield-wf added 2 commits May 6, 2024 11:49

Fix inconsistent formatting

5959e50

Move prerequisite into other boilerplate guide

5cc8d85

greglittlefield-wf commented May 6, 2024

View reviewed changes

greglittlefield-wf changed the title ~~Null-safe props documentation~~ nFED-1885 Null-safe props documentation May 6, 2024

greglittlefield-wf changed the title ~~nFED-1885 Null-safe props documentation~~ FED-1885 Null-safe props documentation May 6, 2024

greglittlefield-wf added 2 commits May 7, 2024 16:46

Update code snippets in doc comments to be null safe

ee9666b

More code snippet null safety updates

405ad4e

greglittlefield-wf marked this pull request as ready for review May 7, 2024 23:50

greglittlefield-wf requested review from aaronlademann-wf and sydneyjodon-wk May 7, 2024 23:50

aaronlademann-wf changed the title ~~FED-1885 Null-safe props documentation~~ FED-1978 Null-safe props documentation May 8, 2024

aaronlademann-wf requested changes May 8, 2024

View reviewed changes

sydneyjodon-wk reviewed May 8, 2024

View reviewed changes

greglittlefield-wf added 2 commits May 10, 2024 14:14

Address CR feedback

3db8659

Expand and clarify prop defaulting options for class components

d14ca46

greglittlefield-wf requested review from aaronlademann-wf and sydneyjodon-wk May 10, 2024 21:42

sydneyjodon-wk approved these changes May 13, 2024

View reviewed changes

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels May 13, 2024

aaronlademann-wf approved these changes May 15, 2024

View reviewed changes

rmconsole-wf added the RM Ready label May 15, 2024

rmconsole-wf approved these changes May 15, 2024

View reviewed changes

rm-astro-wf merged commit 0200369 into master May 15, 2024
7 checks passed

rm-astro-wf deleted the null-safety-docs branch May 15, 2024 18:20

rm-astro-wf mentioned this pull request Apr 26, 2024

RM-250128 RM-250127 Release over_react 5.1.0 #911

Closed

rmconsole-readonly-wk mentioned this pull request May 23, 2024

RM-250128 RM-250127 Release over_react 5.1.0 #919

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FED-1978 Null-safe props documentation #916

FED-1978 Null-safe props documentation #916

greglittlefield-wf commented May 6, 2024 •

edited

Loading

aviary3-wk commented May 6, 2024

greglittlefield-wf May 6, 2024

greglittlefield-wf May 6, 2024 •

edited

Loading

greglittlefield-wf May 6, 2024 •

edited

Loading

aaronlademann-wf left a comment

aaronlademann-wf May 8, 2024

greglittlefield-wf May 10, 2024

aaronlademann-wf May 8, 2024

greglittlefield-wf May 10, 2024

sydneyjodon-wk left a comment

sydneyjodon-wk May 8, 2024

greglittlefield-wf May 10, 2024

sydneyjodon-wk May 8, 2024

greglittlefield-wf May 10, 2024

greglittlefield-wf commented May 10, 2024

sydneyjodon-wk left a comment

aaronlademann-wf left a comment

rmconsole-wf left a comment

		@@ -243,11 +243,17 @@ The use-case for composing multiple props mixins into a single component props c

		#### UiProps as a Map

		> [!WARNING]

		/// UiFactory<FancyInputProps> FancyInput = uiForwardRef(
		/// (props, ref) {


		### Defaulting nullable props using `??`

		In over_react function components, prop defaulting for nullable props is typically implemented using null-aware `??` operators. As a result, unspecified props and explicit `null` values are treated the same.

FED-1978 Null-safe props documentation #916

FED-1978 Null-safe props documentation #916

Conversation

greglittlefield-wf commented May 6, 2024 • edited Loading

Motivation

Changes

Release Notes

Review

QA Checklist

Merge Checklist

aviary3-wk commented May 6, 2024

Security Insights

Action Items

Choose a reason for hiding this comment

greglittlefield-wf May 6, 2024 • edited Loading

Choose a reason for hiding this comment

greglittlefield-wf May 6, 2024 • edited Loading

Choose a reason for hiding this comment

aaronlademann-wf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sydneyjodon-wk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greglittlefield-wf commented May 10, 2024

sydneyjodon-wk left a comment

Choose a reason for hiding this comment

aaronlademann-wf left a comment

Choose a reason for hiding this comment

rmconsole-wf left a comment

Choose a reason for hiding this comment

greglittlefield-wf commented May 6, 2024 •

edited

Loading

greglittlefield-wf May 6, 2024 •

edited

Loading

greglittlefield-wf May 6, 2024 •

edited

Loading