From 7222cd1611b3ee1d1a1abb9ed59bbfb31ff4270f Mon Sep 17 00:00:00 2001 From: Greg Littlefield Date: Fri, 20 Sep 2024 09:49:20 -0700 Subject: [PATCH 1/4] Required prop lint: improve docs, mention edge-cases, add links --- .../src/diagnostic/missing_required_prop.dart | 96 ++++++++++++++----- 1 file changed, 73 insertions(+), 23 deletions(-) diff --git a/tools/analyzer_plugin/lib/src/diagnostic/missing_required_prop.dart b/tools/analyzer_plugin/lib/src/diagnostic/missing_required_prop.dart index 8f249fa2b..02106d23b 100644 --- a/tools/analyzer_plugin/lib/src/diagnostic/missing_required_prop.dart +++ b/tools/analyzer_plugin/lib/src/diagnostic/missing_required_prop.dart @@ -20,45 +20,93 @@ const _desc = r'Always provide required props.'; // const _details = r''' -**ALWAYS** provide a value for `late` required props. +**ALWAYS** provide a value for `late` required props, either directly or by forwarding props. -If a component has a props interface like this: +Please see the documentation for [null safety and required props](https://github.com/Workiva/over_react/blob/master/doc/null_safety_and_required_props.md) +for more information on required prop validation, which, in addition to this lint, also includes runtime `assert`s. -```dart -mixin NavItemProps on UiProps { - bool? isActive; +Those docs also note exceptions to this rule under the [Disabling required prop validation for certain props](https://github.com/Workiva/over_react/blob/master/doc/null_safety_and_required_props.md#disabling-required-prop-validation-for-certain-props) +section, and include instructions for handling those. One common case where this doesn't apply are "wrapper" components that render another component and set some of its props within its render. + +### Examples: + +Given the following component with the required prop `user`: - late void Function() onDidActivate; +```dart +mixin UserChipProps on UiProps { + late User user; + bool? isSelected; } + +UiFactory UserChip = uiFunction((props) { + // ... +}, _$UserChipConfig); ``` -Then the late required prop must always be set by the consumer: +Then whenever UserChip is render, that required prop must always be set by the consumer. **GOOD:** ```dart -@override -render() { - return (NavItem() - ..onDidActivate = () { - // Do something - } - )( - 'Activate me', - ); -} + (UserChip()..user = user)() ``` **BAD:** ```dart -@override -render() { - return NavItem()( - 'You probably cannot activate me :(', - ); + UserChip()() +// ^^^^^^^^^^ +// warning: Missing required late prop 'user' from 'UserChipProps'. +// (over_react_late_required_prop) +``` +and, that code will also throw a runtime error when asserts are enabled: + +``` +Uncaught Error: RequiredPropsError: Required prop `user` is missing. + at Object.throw_ [as throw] + at _$$UserChipProps$JsMap.new.validateRequiredProps +``` + +#### Prop forwarding + +**GOOD:** +```dart +mixin CustomUserChipPropsMixin on UiProps { + String? color; } + +class CustomUserChipProps = UiProps with UserChipProps, CustomUserChipPropsMixin; + +UiFactory CustomUserChip = uiFunction((props) { + final color = props.color; + + return (UserChip() + // Required props are correctly forwarded here by the wrapper component + ..addProps(props.getPropsToForward(exclude: {CustomUserChipPropsMixin}) + ..style = { + if (color != null) 'border': '2px solid $color', + ...?props.style, + } + )(); +}, _$CustomUserChipConfig); ``` +**BAD:** +```dart +UiFactory CustomUserChip = uiFunction((props) { + final color = props.color; + + // Required props are not forwarded, so we get: + // warning: Missing required late prop 'user' from 'UserChipProps'. + // (over_react_late_required_prop) + return (UserChip() + ..style = { + if (color != null) 'border': '2px solid $color', + ...?props.style, + } + )(); +}, _$CustomUserChipConfig); +``` '''; + // class MissingRequiredPropDiagnostic extends ComponentUsageDiagnosticContributor { @@ -82,7 +130,9 @@ class MissingRequiredPropDiagnostic extends ComponentUsageDiagnosticContributor ); static const _correctionMessage = - "Either set this prop, or mix it into the enclosing component's props and forward it."; + "Either set this prop, or mix it into the enclosing component's props and forward it." + " Or, if this is a wrapper component that sets this prop within its render," + " disable validation for this prop (see instructions in documentation link)."; static DiagnosticCode _codeForRequiredness(PropRequiredness requiredness) { switch (requiredness) { From 7e7dda5806f28dbb10b8f87b94fa0901c728ef16 Mon Sep 17 00:00:00 2001 From: Greg Littlefield Date: Fri, 20 Sep 2024 10:26:08 -0700 Subject: [PATCH 2/4] Add docs around connect and late props, migration info for connect and wrappers --- doc/null_safety/null_safe_migration.md | 23 ++++++++++++++++ doc/null_safety_and_required_props.md | 37 +++++++++++++++++++++++++- doc/over_react_redux_documentation.md | 18 +++++++++++++ 3 files changed, 77 insertions(+), 1 deletion(-) diff --git a/doc/null_safety/null_safe_migration.md b/doc/null_safety/null_safe_migration.md index bdc2ad3c4..7429bd1e6 100644 --- a/doc/null_safety/null_safe_migration.md +++ b/doc/null_safety/null_safe_migration.md @@ -138,6 +138,14 @@ Run the null safety migrator tool: Below are some common cases that might come up while running the migrator tool on a repo using over_react. +* [Prop requiredness and nullability](#prop-requiredness-and-nullability) +* [Wrapper and `connect`ed components and required props](#wrapper-and-connected-components-and-required-props) +* [Implementing abstract `Ref`s](#implementing-abstract-refs) +* [Incorrect `getDerivedStateFromProps` Return Signature](#incorrect-getderivedstatefromprops-return-signature) +* [Verbose function component return signature for `uiForwardRef`](#verbose-function-component-return-signature-for-uiforwardref) +* [Nullable store/actions generics on `FluxUiPropsMixin`](#nullable-storeactions-generics-on-fluxuipropsmixin) +* [Nullable props generic on `UiComponent2` mixins](#nullable-props-generic-on-uicomponent2-mixins) + #### Prop requiredness and nullability First, check out our documentation around [null safety and required props](../null_safety_and_required_props.md). @@ -188,6 +196,21 @@ flowchart TD End_Required[/"Make it required\nlate SomeType propName;"\] ``` +### Wrapper and `connect`ed components and required props + +There may be some cases where you have a wrapper component or `connect`ed component that sets some required props, +but you get lints and runtime errors about missing props when consuming them. + +See [this section](../null_safety_and_required_props.md#disabling-required-prop-validation-for-certain-props) +of the null safety and required props docs for instructions on how to handle these cases and suppress this validation. + +#### connect + +We'll be adding a codemod to help migrate the `connect` case: https://github.com/Workiva/over_react_codemod/issues/295 + +Alternatively, you could refactor your component to instead utilize [OverReact Redux hooks](./over_react_redux_documentation.md#hooks), +which avoid this problem by accessing store data and dispatchers directly in the component as opposed to passing it in via props. + #### 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: diff --git a/doc/null_safety_and_required_props.md b/doc/null_safety_and_required_props.md index 99da4a6e8..602d35102 100644 --- a/doc/null_safety_and_required_props.md +++ b/doc/null_safety_and_required_props.md @@ -120,8 +120,9 @@ This mechanism does not apply to function components, which use a different prop Sometimes, you want to declare a prop as non-nullable and required, but not enforce that consumers explicitly set it. -There are two ways to opt out of prop validation for certain props, targeted toward to main use-cases: +There are two ways to opt out of prop validation for certain props, targeted toward these main use-cases: - [wrapper components](#disabling-validation-use-case-wrapper-components) +- [connect](#disabling-validation-use-case-connect) - [cloned props](#disabling-validation-use-case-cloned-props) #### Disabling validation use-case: wrapper components @@ -180,6 +181,40 @@ class WrapperProps = UiProps with FooProps, WrapperPropsMixin; > See the [unsafe required prop reads](#unsafe-required-prop-reads) section for more info +#### Disabling validation use-case: `connect` +Similar to [the wrapper component case](#disabling-validation-use-case-wrapper-components) in the previous section, +we'll want to disable validation similarly using `@Props(disableRequiredPropValidation: {...})` +for any late required props assigned within connect. + +For example: +```dart +mixin CounterPropsMixin on UiProps { + // Set in connect. + late int count; + late void Function() increment; + + // Must be set by consumers of the connected compoennt. + late String requiredByConsumer; +} + +@Props(disableRequiredPropValidation: {'count', 'increment'}) +class CounterProps = UiProps with CounterPropsMixin, OtherPropsMixin; + +UiFactory Counter = connect( + mapStateToProps: (state) => (Counter() + ..count = state.count + ), + mapDispatchToProps: (dispatch) => (Counter() + ..increment = (() => dispatch(IncrementAction())) + ), +)(_$Counter); + +example() => (Counter()..requiredByConsumer = 'foo')(); +``` + +Note that [OverReact Redux hooks](./over_react_redux_documentation.md#hooks) +avoid this problem by accessing store data and dispatchers directly in the component as opposed to passing it in via props. + #### Disabling validation use-case: cloned props Sometimes, you want to declare a prop that's always cloned onto it by a parent component. diff --git a/doc/over_react_redux_documentation.md b/doc/over_react_redux_documentation.md index 232096c02..52d6e9eb6 100644 --- a/doc/over_react_redux_documentation.md +++ b/doc/over_react_redux_documentation.md @@ -181,6 +181,24 @@ UiFactory Counter = connect( )(_$Counter); ``` +Note that any required props assigned in connect must have their validation disabled; see docs [here](./null_safety_and_required_props.md#disabling-validation-use-case-connect) +for more info. + +For example: +```dart +mixin CounterPropsMixin on UiProps { + // Set in connect. + late int count; + late void Function() increment; + + // Must be set by consumers of the connected compoennt. + late String requiredByConsumer; +} + +@Props(disableRequiredPropValidation: {'count', 'increment'}) +class CounterProps = UiProps with CounterPropsMixin, OtherPropsMixin; +``` + ### `connect` Parameters - #### `mapStateToProps` From e7c843022475e8e42d9e44bf2722ece990ec7336 Mon Sep 17 00:00:00 2001 From: Greg Littlefield Date: Fri, 20 Sep 2024 10:36:57 -0700 Subject: [PATCH 3/4] Make required props error more helpful --- lib/src/component_declaration/builder_helpers.dart | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/lib/src/component_declaration/builder_helpers.dart b/lib/src/component_declaration/builder_helpers.dart index 74fd6c9d3..0192450d1 100644 --- a/lib/src/component_declaration/builder_helpers.dart +++ b/lib/src/component_declaration/builder_helpers.dart @@ -158,8 +158,13 @@ class MissingRequiredPropsError extends Error { MissingRequiredPropsError(this._message); + static const _messageSuffix = ' Ensure this prop is either directly set, or indirectly set via prop forwarding.' + 'If this error seems unexpected and this component uses connect or mixes in required props from another component,' + ' please refer to the null safety migration guide for instructions on how to proceed:' + ' https://github.com/Workiva/over_react/blob/master/doc/null_safety/null_safe_migration.md#wrapper-and-connected-components-and-required-props'; + @override - String toString() => 'RequiredPropsError: $_message'; + String toString() => 'RequiredPropsError: $_message$_messageSuffix'; } /// Helper static extension methods to make forwarding props easier. From 64c00a088908bff022005ac586c6b7201f4b5eeb Mon Sep 17 00:00:00 2001 From: Greg Littlefield Date: Mon, 23 Sep 2024 10:15:58 -0700 Subject: [PATCH 4/4] Address CR feedback --- doc/null_safety/null_safe_migration.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/doc/null_safety/null_safe_migration.md b/doc/null_safety/null_safe_migration.md index 7429bd1e6..a1c756fbd 100644 --- a/doc/null_safety/null_safe_migration.md +++ b/doc/null_safety/null_safe_migration.md @@ -206,10 +206,11 @@ of the null safety and required props docs for instructions on how to handle the #### connect -We'll be adding a codemod to help migrate the `connect` case: https://github.com/Workiva/over_react_codemod/issues/295 - -Alternatively, you could refactor your component to instead utilize [OverReact Redux hooks](./over_react_redux_documentation.md#hooks), -which avoid this problem by accessing store data and dispatchers directly in the component as opposed to passing it in via props. +For connect, either +- Disable validation using the instructions linked above + - Note: for now, this must be done manually, but we'll be adding a codemod to help do this automatically for `connect`: https://github.com/Workiva/over_react_codemod/issues/295 +- Refactor your component to instead utilize [OverReact Redux hooks](../over_react_redux_documentation.md#hooks), + which avoid this problem by accessing store data and dispatchers directly in the component as opposed to passing it in via props. #### Implementing abstract `Ref`s