Make adjustments necessary to support strong-mode compliance for over_react #106
Conversation
+ So that over_react can override them with strong typing + Ref: Workiva/over_react#14
| @@ -5,14 +5,28 @@ | |||
| /// A Dart library for building UI using ReactJS. | |||
| library react; | |||
|
|
|||
| import 'dart:html' show Element; | |||
There was a problem hiding this comment.
lib/react.dart is also used for server-side rendering, and should not import dart:html
| /// ReactJS `Component` props. | ||
| Map props; | ||
| Map get props => _props; | ||
| set props(Map value) => _props = value; |
There was a problem hiding this comment.
As opposed to making these getters/setters, can we just use the @virtual annotation instead? (See meta package 1.0.4 changelog)
There was a problem hiding this comment.
@greglittlefield-wf I just tried that, and I still get the following errors in over_react:
ERROR: Field declaration Component.ref cannot be overridden in UiComponent. ([over_react] lib/src/component_declaration/component_base.dart:106)
ERROR: Field declaration Component.props cannot be overridden in UiComponent. ([over_react] lib/src/component_declaration/component_base.dart:179)
ERROR: Field declaration Component.state cannot be overridden in UiStatefulComponent. ([over_react] lib/src/component_declaration/component_base.dart:230)
There was a problem hiding this comment.
I believe you will need to be using Dart 1.19.1.
There was a problem hiding this comment.
Actually just tried this, still does not work. I found this issue dart-lang/sdk#27452, which makes it seem like strong mode does not allow you to override fields, even with the @virtual annotation. I think that may just be for overriding fields with other fields.
There was a problem hiding this comment.
That's too bad. Can we add a code comment, then, explaining that this is for strong mode compliance and adding a TODO linked to that SDK issue?
There was a problem hiding this comment.
@greglittlefield-wf good idea.
| @@ -13,7 +13,8 @@ int _nextFactoryId = 0; | |||
| /// | |||
| /// This prevents React JS from not printing key warnings it deems as "duplicates". | |||
| void renderWithUniqueOwnerName(ReactElement render()) { | |||
| ReactDartComponentFactoryProxy factory = react.registerComponent(() => new _OwnerHelperComponent()); | |||
| ReactDartComponentFactoryProxy factory = | |||
There was a problem hiding this comment.
#nit I think in strong mode the LHS typing is unnecessary with this cast. Also, unnecessary parentheses? Could do:
final factory =
react.registerComponent(() => new _OwnerHelperComponent()) as ReactDartComponentFactoryProxy.
|
@greglittlefield-wf all feedback addressed except for using the |
| dynamic ref; | ||
| /// Returns the component of the specified [ref]. | ||
| /// | ||
| /// Returns a [Component] if it is a Dart component, otherwise returns an "Dom component" (Dart `Element`). |
There was a problem hiding this comment.
#nit Doc comments for getters should be noun phrases: https://www.dartlang.org/guides/language/effective-dart/documentation#prefer-starting-variable-getter-or-setter-comments-with-noun-phrases
Not sure why the comment is referencing the getter itself—doesn't really make sense.
Also, this is missing the case for JS components.
What about:
/// A function that, when called with a String ref, returns the associated component.
///
/// This function will return either:
///
/// * [Component] for Dart components
/// * `ReactElement` for JS composite components
/// * `Element` DOM components| /// Returns the component of the specified [ref]. | ||
| /// | ||
| /// Returns a [Component] if it is a Dart component, otherwise returns an "Dom component" (Dart `Element`). | ||
| dynamic get ref => _ref; |
There was a problem hiding this comment.
While we're in here, can we make the type more specific, as we did in over_react?
typedef dynamic _RefTypedef(String ref);
_RefTypedef _ref;
_RefTypedef get ref => _ref;
set ref(_RefTypedef value) => _ref = value;There was a problem hiding this comment.
@greglittlefield-wf I wasn't sure if that typedef should live here... had it moved over at one point. Will move it back.
+ Since their redirects aren’t exactly precise.
|
+1 |
|
@greglittlefield-wf ready for a final pass. |
| Map get state => _state; | ||
| set state(Map value) => _state = value; | ||
|
|
||
| Ref get ref => _ref; |
There was a problem hiding this comment.
These getters should have doc comments, since consumers can't see the doc comments of private fields.
What about this:
/// A private field that backs [ref], which is exposed via getter/setter so
/// it can be overridden in strong mode.
///
/// Necessary since the `@virtual` annotation within the meta package
/// [doesn't work for overriding fields](https://github.com/dart-lang/sdk/issues/27452).
///
/// TODO: Switch back to a plain field once this issue is fixed.
Ref _ref;
...
/// A function that returns a component reference:
///
/// * [Component] if it is a Dart component.
/// * `Element` _(DOM node)_ if it is a React DOM component.
Ref get ref => _ref;
set ref(Ref value) => _ref = value;+ Leaving only the TODO portion on the private field.
|
@greglittlefield-wf feedback addressed |
|
+1, awesome stuff! |
|
@trentgrover-wf this is ready for a merge + tag. |
In order for
over_reactto become strong mode compliant, a few changes are necessary in the wrapper:react.Component:propsandstatefields in-favor of private fields with public getters and setters.over_reactcan add strong typing topropsandstatewithinUiComponent/UiStatefulComponent, respectively without generating Dart analyzer errors.MapI also cleaned up a few analyzer errors showing in this repo as well.
FYA @greglittlefield-wf @jacehensley-wf @clairesarsam-wf @trentgrover-wf