Open dave-doty opened 1 week ago
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
).
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.
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.
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).
However, it currently only warns about component usages that have been fully invoked to create ReactElement
s; 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:
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.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.
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 propmouseover_datas
is not being specified, but that does not show up: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:"
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.