From 43e2ff7b620a11601ccb29daf4d2ebececb3297c Mon Sep 17 00:00:00 2001 From: Aaron Lademann Date: Tue, 16 Jul 2024 16:36:47 -0700 Subject: [PATCH] Update the null safety migration documentation --- doc/null_safety/null_safe_migration.md | 182 +++++++++++++++++++++++-- 1 file changed, 167 insertions(+), 15 deletions(-) diff --git a/doc/null_safety/null_safe_migration.md b/doc/null_safety/null_safe_migration.md index 78cb8c8b2..ad00b84ae 100644 --- a/doc/null_safety/null_safe_migration.md +++ b/doc/null_safety/null_safe_migration.md @@ -6,8 +6,9 @@ Steps to migrate a repo to null safety: 1. [View resources](#step-1-view-resources) -1. [Run the preparation codemod](#step-2-run-the-preparation-codemod-coming-soon) -1. [Run the migration tool](#step-3-run-the-migration-tool) +1. [Run the preparation codemods](#step-2-run-the-preparation-codemods) +1. [Enable the analyzer plugin](#step-3-enable-the-analyzer-plugin) +1. [Run the migration tool](#step-4-run-the-migration-tool) ## Step 1: View resources @@ -16,29 +17,76 @@ If you haven't already, familiarize yourself with the concepts of Dart null safe - Dart's [Understanding null safety](https://dart.dev/null-safety/understanding-null-safety) article - OverReact's documentation on [null safety and required props](../null_safety_and_required_props.md) -## Step 2: Run the preparation codemod (coming soon!) +## Step 2: Run the preparation codemods - - -> [!WARNING] -> This codemod does not exist yet, but is coming soon! - -Start out by running the `null_safety_prep` [codemod][orcm] on your repo: +Start out by running the [codemod][orcm]s on your repo: ``` dart pub global activate over_react_codemod -dart pub global run over_react_codemod:null_safety_prep +dart pub global run over_react_codemod:null_safety_prep --yes-to-all +dart pub global run over_react_codemod:null_safety_migrator_companion --yes-to-all ``` -This codemod will: +These codemods will: - Migrate as many over_react specific use cases as possible. - Get the repo into a state where the migration tool can be run with less manual intervention. - -## Step 3: Run the migration tool +- Add nullability hints to props/state that are defaulted/initialized in class components. + - These hints will cause defaulted/initialized values to be migrated as "late required". + See our [prop nullability](#prop-nullability) docs for more details on whether you should keep them required following the migration. + +## Step 3: Enable the analyzer plugin + +Enabling the analyzer plugin will help you spot potential null-safety issues in your components at analysis time, +especially as it pertains to [prop nullability](#prop-nullability). + +1. Enable the plugin in your `analysis_options.yaml` file: + ```yaml + analyzer: + plugins: + - over_react + ``` +1. Restart the Dart Analysis Server in your IDE. The plugin may take a minute to load after built-in analysis completes. + + * If the number of new lints / errors is overwhelming, you an also use this snippet to disable all lints except for the ones that are particularly useful during a null-safety migration: + ```yaml + analyzer: + errors: + over_react_exhaustive_deps: ignore + over_react_invalid_render_return_type: ignore + over_react_pseudo_static_lifecycle: ignore + over_react_rules_of_hooks: ignore + over_react_style_missing_unit: ignore + over_react_avoid_link_target_vulnerability: ignore + over_react_boilerplate_warning: ignore + over_react_create_ref_usage: ignore + over_react_forward_only_dom_props_to_dom_builders: ignore + over_react_hash_code_as_key: ignore + over_react_invalid_dom_attribute: ignore + over_react_low_quality_key: ignore + over_react_missing_cascade_parens: ignore + over_react_missing_key: ignore + over_react_object_to_string_as_key: ignore + over_react_proptypes_do_not_throw: ignore + over_react_unknown_key_type: ignore + over_react_boilerplate_debug: ignore + over_react_bool_prop_name_readability: ignore + over_react_consumed_props_return_value: ignore + over_react_incorrect_doc_comment_location: ignore + over_react_prefer_null_over_false_render_return_type: ignore + over_react_prefer_use_or_create_ref: ignore + over_react_string_ref: ignore + over_react_unnecessary_key: ignore + over_react_variadic_children: ignore + plugins: + over_react + ``` + +## Step 4: Run the migration tool Run the null safety migrator tool: -- For Workiva employees, use the [Workiva version of the migrator tool](https://github.com/Workiva/wnnbd?tab=readme-ov-file#migrating-a-package). +- **For Workiva employees, use the [Workiva version of the migrator tool](https://github.com/Workiva/wnnbd?tab=readme-ov-file#migrating-a-package).** + - Note that there may be additional preparation steps recommended in the documentation for that tool. - Otherwise, either: - use [Dart's migrator tool](https://dart.dev/null-safety/migration-guide#migration-tool) - use [a fork of the migrator tool](#migrator-tool-bug) that fixes a bug with how over_react usages are migrated @@ -68,7 +116,111 @@ Below is a table of the possible options for prop nullability: - **Nullable**: If the prop is required, but can be explicitly set to `null`. - **Non-nullable**: If the prop is required and should never be set to `null`. -#### Migrator tool bug +If you aren't sure whether the prop should be nullable, or required, or both, this decision tree might help: + +```mermaid +--- +title: Prop Nullability Decision Tree +--- +flowchart TD + Start[Should My Prop By Required Or Optional]==>HasDefault + HasDefault((Does the \nprop have a \ndefault value?))== Yes ==> Defaulted + Defaulted((Where is \nthe default?))-- defaultProps getter\n(Class Component) --> End_Defaulted1[/"Make it required\nlate SomeType propName;"\] + Defaulted-- local var\n(Function Component) --> End_Optional1[/"Make it optional\nSomeType? propName;"\] + HasDefault == No ==> NotDefaulted + NotDefaulted((Is it set \nfor every \ninvocation?))-- No --> End_Optional2[/"Make it optional\nSomeType? propName;"\] + NotDefaulted-- Yes --> AlwaysSpecified + AlwaysSpecified((Is the prop \npublic API?))-- Yes --> PublicAlwaysSpecified + PublicAlwaysSpecified((Is the prop mixed in \nby any other component?))-- Yes --> End_PublicAlwaysSpecifiedMixedIn[/"Make it optional\nSomeType? propName;"\] + PublicAlwaysSpecified-- No --> End_PublicAlwaysSpecifiedNotMixedIn[/"Make it required\nlate SomeType propName;"\] + AlwaysSpecified-- No --> PublicAlwaysSpecifiedNotConsumed[/"Make it required\nlate SomeType propName;"\] +``` + +#### Implementing abstract `Ref`s + +After migrating to null-safety, it may be necessary to add left side typing on `Ref`s when overriding abstract getters as shown in the example below: + +```dart +// in some_abstract_component_or_mixin +abstract class AbstractFoo extends UiComponent2 { + Ref get someRef; +} + +// in impl +class Foo extends AbstractFoo { + // Incorrectly inferred as Ref + @override + final someRef = createRef(); + + // Correctly typed as Ref + @override + final Ref someRef = createRef(); +} +``` + +#### Incorrect `getDerivedStateFromProps` Return Signature + +The migrator often incorrectly makes the return signature of `getDerivedStateFromProps` non-nullable. + +The correct null-safe signature for this class component method is: + +```dart +Map? getDerivedStateFromProps(Map nextProps, Map prevState) {} +``` + +#### Verbose function component return signature for `uiForwardRef` + +If you are migrating `uiForwardRef` components that lack an explicit return signature on the left side like this: + +```dart +final SomeComponent = uiForwardRef(/*...*/); +``` + +the migrator tool will add some very verbose left side typing based on type inference. **Changes like this are safe to discard.** + +```dart +final SomeComponentProps Function([Map]) SomeComponent = uiForwardRef(/*...*/); +``` + +#### Nullable store/actions generics on `FluxUiPropsMixin` + +The migrator tool attempts to infer the nullability of `FluxUiPropsMixin.store` / `FluxUiPropsMixin.actions` based +on usage - which can lead to the type generics being incorrectly migrated as nullable types. + +**These generics should always be set with non-nullable types.** + +```dart +// Incorrect +class FooProps = UiProps with FluxUiPropsMixin; + +// Correct +class FooProps = UiProps with FluxUiPropsMixin; +``` + +#### Nullable props generic on `UiComponent2` mixins + +When you have a component mixin that contains a generic which has a base type of a props mixin like so: + +```dart +mixin FooProps on UiProps {/*...*/} + +mixin Foo on UiComponent2 {/*...*/} +``` + +The migrator tool sometimes incorrectly infers `UiComponent2.props` as nullable like so: + +```dart +mixin FooProps on UiProps {/*...*/} + +// Incorrect. FooProps? should be non-nullable FooProps +mixin Foo on UiComponent2 {/*...*/} +``` + +**Generic props applied to the `UiComponent2` constraint should always be non-nullable.** + +### Migrator Bugs + +#### Migrator treats emulated function calls as nullable The migrator tool [incorrectly treats the result of emulated function calls as nullable](https://github.com/dart-lang/sdk/issues/46263).