over_react analyzer not reporting at compile time required props that are missing, and at runtime, reporting error on required props that are specified

[dave-doty]

• Issue Type: BUG
• over_react Version(s): 5.3.0
• dart version 2.19.6

I got the analyzer plugin set up, and it is indeed showing some warnings in WebStorm, so it's running:

However, the main bug I had with pre-null-safe overreact was forgetting to include a required prop and having it default to null. I had really hoped to be warned about this (in fact it seems like the severity should default to error), but in the following code, I have commented out the line // ..mouseover_datas = state.ui_state.mouseover_datas to see if the analyzer plugin will warn me that the required prop mouseover_datas is not being specified, but that does not show up:

UiFactory<DesignFooterProps> ConnectedDesignFooter = connect<AppState, DesignFooterProps>(
  mapStateToProps: (state) {
    BuiltList<MouseoverData> mouseover_datas = state.ui_state.mouseover_datas;
    MouseoverData? first_mouseover_data =
        mouseover_datas.isNotEmpty ? state.ui_state.mouseover_datas.first : null;
    Strand? strand_first_mouseover_data =
        mouseover_datas.isNotEmpty ? state.design.substrand_to_strand[first_mouseover_data!.domain] : null;
    String loaded_filename = state.ui_state.loaded_filename;
    return (DesignFooter()
      // ..mouseover_datas = state.ui_state.mouseover_datas // This should be an error since `mouseover_datas` is declared as `late` below
      ..strand_first_mouseover_data = strand_first_mouseover_data
      ..loaded_filename = loaded_filename);
  },
)(DesignFooter);

UiFactory<DesignFooterProps> DesignFooter = _$DesignFooter;

mixin DesignFooterProps on UiProps {
  late BuiltList<MouseoverData> mouseover_datas;
  Strand? strand_first_mouseover_data;
  late String loaded_filename;
}

class DesignFooterComponent extends UiComponent2<DesignFooterProps> {
  @override
  render() {
    BuiltList<MouseoverData> mouseover_datas = props.mouseover_datas;
    ...

I did try restarting the analyzer server by clicking the red curved arrows in the upper left, but still nothing about this error.

Is there something I need to configure in the overreact analyzer to ask it to warn me about missing required props?

I noticed that this page mentions some customization:

"To configure the plugin, add a over_react key to analysis_options.yaml:"

analyzer:
  plugins:
    - over_react

over_react:
  errors:
    over_react_boilerplate_error: error
    over_react_incorrect_doc_comment_location: warning
    over_react_unnecessary_key: info
    over_react_pseudo_static_lifecycle: ignore

But I can't find any documentation of what all the options are for the analyzer, and what "over_react_missing_required_prop" would actually be.

FYI: @greglittlefield-wf @aaronlademann-wf @kealjones-wk @evanweible-wf @maxwellpeterson-wf

[dave-doty]

While I'm at it, there's another strange (and more serious) bug, where I am being told I missed a required prop even though I did specify it:

Although I am providing a non-null mouseover_datas (when I uncomment the line // ..mouseover_datas = ... above) and loaded_filename, I get this error when I run the application:

errors.dart:263 Uncaught (in promise) DartError: RequiredPropsError: Required prop `mouseover_datas` is missing.
    at Object.throw_ [as throw] (errors.dart:263:21)
    at design_footer._$$DesignFooterProps$JsMap.new.validateRequiredProps (design_footer.over_react.g.dart:227:7)
    at component_base.dart:570:9
    at [_sharedAsserts] (component_base.dart:572:14)
    at design_footer._$$DesignFooterProps$JsMap.new.call (component_base.dart:636:5)
    at design.DesignViewComponent.new.render (design.dart:897:36)
    at DevToolsMiddleware.new.load_file_middleware (load_file.dart:39:18)
    at DevToolsMiddleware.new.call (middleware.dart:39:25)
    at store.dart:255:43
    at middleware.dart:35:20
    at DevToolsMiddleware.new.save_file_middleware (save_file.dart:11:7)
    at DevToolsMiddleware.new.call (middleware.dart:39:25)
    at store.dart:255:43
    at middleware.dart:35:20
    at DevToolsMiddleware.new.export_svg_middleware (export_svg.dart:77:9)
    at DevToolsMiddleware.new.call (middleware.dart:39:25)
    at store.dart:255:43
    at middleware.dart:35:20
    at DevToolsMiddleware.new.forbid_create_circular_strand_no_crossovers_middleware$ (forbid_create_circul…iddleware.dart:87:7)
    at DevToolsMiddleware.new.call (middleware.dart:39:25)
    at store.dart:255:43
    at middleware.dart:35:20
    at DevToolsMiddleware.new.move_ensure_all_in_same_helix_group_middleware (move_ensure_same_group.dart:21:7)
    at DevToolsMiddleware.new.call (middleware.dart:39:25)
    at store.dart:255:43
    at middleware.dart:35:20
    at DevToolsMiddleware.new.local_storage_middleware (local_storage.dart:130:7)
    at DevToolsMiddleware.new.call (middleware.dart:39:25)
    at store.dart:255:43
    at middleware.dart:35:20
    at DevToolsMiddleware.new.reset_local_storage_middleware (reset_local_storage.dart:19:7)
    at DevToolsMiddleware.new.call (middleware.dart:39:25)
    at store.dart:255:43
    at Store.new.dispatch (store.dart:267:25)
    at DevToolsStore.new.dispatch (store.dart:74:14)
    at load_file.dart:23:20
    at future.dart:424:39
    at internalCallback (isolate_helper.dart:48:19)

I added a statement print("mouseover_datas: ${mouseover_datas}"); just before the return statement in the mapStateToProps callback at the top, but I don't see the print statement appear before the error. The stack trace points to my code at line (design.dart:897:36) as causing the validation error, which is where I set up the root of the React tree for this connected component:

react_dom.render(
  over_react_components.ErrorBoundary()(
    (ReduxProvider()..store = app.store)(
      ConnectedDesignFooter()(),  // this is the line corresponding to (design.dart:897:36)
    ),
  ),
  this.footer_element,
);

I can fix that error by declaring that mouseover_datas is not required: BuiltList<MouseoverData>? mouseover_datas; in the props mixin, and then using props.mouseover_datas! in the render() method, but then I get the same error for the prop loaded_filename. I can fix both errors by declaring they are both nullable and then using ! to assert they are not null. But of course this is not a solution, because I want OverReact to properly tell me when a required prop is missing, so pretending that none of my props are required is not a good fix.

I haven't migrated very many of my react components to null safety yet, but the difference between this and the others I have migrated is that this is a connected component, whereas the others were rendered by a parent react component.

Are connected components treated differently? It's as though OverReact is trying to render the component once before actually calling the mapStateToProps callback, since this error is occurring before that gets called (as evidenced by the lack of a print statement occurring before the error when I add a print statement to mapStateToProps).

[dave-doty]

For anyone stumbling on this issue looking for the over_react analyzer options that can be specified in analysis_options.yaml as suggested here, I found the list of options: https://workiva.github.io/over_react/analyzer_plugin/lints/

I guessed that putting this in my analysis_options.yaml:

over_react:
  errors:
    over_react_required_props: error

would cause OverReact to warn about required props that were not given when creating a component, but so far I haven't seen that error show up even when I try to trigger it.

[dave-doty]

I figured out a workaround for this error. For a connected component (unlike in non-null-safe-mode, i.e., if you don't have // @dart=2.9 at the top of the file), you have to set the required props both in the mapStateToProps in the React-Redux connect function (or related like mapStateToPropsWithOwnProps):

UiFactory<DesignLoadingDialogProps> ConnectedLoadingDialog = connect<AppState, DesignLoadingDialogProps>(
  mapStateToProps: (state) =>
    DesignLoadingDialog()..show = state.ui_state.show_load_dialog;
)(DesignLoadingDialog);

and when you hook up the React tree to the DOM:

render_loading_dialog() {
  react_dom.render(
    over_react_components.ErrorBoundary()(
      (ReduxProvider()..store = app.store)(
        (ConnectedLoadingDialog()..show = state.ui_state.show_load_dialog)(),
      ),
    ),
    this.dialog_loading_container,
  );
}

This seems redundant. I am writing some code to help avoid repeating the same code over and over for each connected component (most of mine have many more props than one, so I don't want to have so much duplicated code). But I assume this is not the intended usage; if anyone from the team can point me to a better way to handle this in null-safe code, I'd appreciate it.

[greglittlefield-wf]

Hey @dave-doty!

The over_react analyzer plugin should be warning about required props, under the code over_react_missing_required_props, by default.

If you're seeing that not working, you could try the following steps to debug it (first, though, see my notes below about fully-invoked usages and connect).

Click to expand debugging steps

1. Ensure the analyzer plugin is enabled (looks like it is because it's running for you)
2. Double-check that your IDE is using the same Dart version you're using locally (in IntelliJ/WebStorm, this is in settings under "Languages & Frameworks > Dart"
3. Check that the sure the analyzer plugin is running and using over_react 5.x, by:
   1. Opening the Dart analyzer diagnostics page
      
   2. Going to the "Plugins tab" and verifying the over_react plugin is present and has a 5.x version:
      
4. If the plugin version does not match, ensure over_react is resolved to the latest version in each package you have open in your project folder
5. Try Excluding analysis for any nested packages you may have; nested packages that aren't excluded can interfere with the plugin reporting lints properly
6. If all else fails and the plugin seems to be running on the right version, try:
   1. Closing your IDE and any Dart web servers
   2. Deleting the .dart_tool directory for your package(s), deleting the $HOME/.dartServer/ directory
   3. Rynning a pub get in your package, opening your IDE, and trying again

However, it currently only warns about component usages that have been fully invoked to create ReactElements; so, for example:

// Should warn about missing required props
(DesignFooter()
   // ..mouseover_datas = state.ui_state.mouseover_datas
)()
// Will not warn about missing required props
(DesignFooter()
  // ..mouseover_datas = state.ui_state.mouseover_datas
)

Generally, it's because there are cases where you might want to construct a map that contains a "partial" set of props, and we found a lot of existing code using that pattern that would be broken if we linted for those cases.

We could potentially add a special-case to lint for the return value of mapStateToProps in usages of connect, but we'd have to be able to tell which required props are supposed to be provided by connect, vs which required props are supposed to be provided by the consumers of that component.

That leads us into the runtime error you're hitting, and having to set the required prop both in mapStateToProps and when rendering the connected component.

The returned connected component uses the same props implementation as the underlying component, so it still validates at runtime that all required props are set, since at that point in time mapStateToProps hasn't run yet and they haven't been applied. It seems we missed thinking about how that case would work; sorry about that, and thanks for bringing it to our attention!

We'll have to think a little more on what the best way to deal with that case is, but in the meantime, two potential solutions come to mind:

• For required props on connected components that are supposed to be wired up in mapStateToProps, disable required prop validation for each of them. That should fix the runtime errors you're getting, and also make the props not lint when you're consuming the connected component.
• Instead of using connect, use React Redux hooks, which don't require declaring props when selecting values. However, this requires switching over to function components, and can be a bit of an involved refactor depending on your components.

[greglittlefield-wf]

We weren't able to come up with any other recommendations, so docs have been updated here and elsewhere to reflect that (see this PR for full docs changes: #949).

[dave-doty]

Regarding the original issue that analyzer was not reporting when I failed to specify a required prop, I now see that it does correctly report this.

I suspect the issue was that I was migrating the components one-by-one to null safety, and perhaps there was some issue where if one of the parent or child components was still not null-safe (i.e., had the // @dart=2.9 at the top of the file), then the analyzer was not checking. Now that I've migrated all of my components, if I go and comment out a setting of a required prop, I do in fact see the warning.

But if there is something to my guess, you might consider adding that to the migration guide, that the warnings only appear if all components involved lack the line // @dart=2.9.

[dave-doty]

• Instead of using connect, use React Redux hooks, which don't require declaring props when selecting values. However, this requires switching over to function components, and can be a bit of an involved refactor depending on your components.

Would you mind fleshing out the hook examples a bit more? For instance there's this at the page you linked:

import 'package:over_react/over_react_redux.dart';
import 'package:over_react/react_dom.dart' as react_dom;

main() {
  react_dom.render(
      (ReduxProvider()..store = yourReduxStoreInstance)(
        // Function components that use the hooks in this section go here.
      ), 
      querySelector('#id_of_mount_node'));
}

but I don't know what to replace // Function components that use the hooks in this section go here. with.

Is there a complete, self-contained example somewhere for how to use functional components with Redux?

Thanks so much.

[greglittlefield-wf]

I suspect the issue was that I was migrating the components one-by-one to null safety, and perhaps there was some issue where if one of the parent or child components was still not null-safe (i.e., had the // @Dart=2.9 at the top of the file), then the analyzer was not checking.

Ah, sorry about that; I wonder if it was caused by this bug #948, fixed in over_react 5.3.2? We had only discovered that after I responded to this thread, and I hadn't thought about how it might impact this.

Is there a complete, self-contained example somewhere for how to use functional components with Redux?

A little further in on that section, the useSelector and useDispatch sections show more complete examples of those function components.

We also have an example here (not-ideally, hidden in the web directory): https://github.com/Workiva/over_react/blob/master/web/over_react_redux/examples/simple/components/counter.dart#L21-L37