-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UIP-1637 First pass at strong mode compliance #15
Changes from 20 commits
27c1a59
f2a5cc6
f1c6657
17f16fa
2804edf
d9071f8
af8d5f9
2914187
3b51626
0e7c44a
c2299be
fc1b96e
b76c072
fe69d03
46639f2
047a8a9
9d6a136
0fe7556
2507729
1b4a80a
d205ea9
5547f8d
c4dd932
f211b71
ba742e8
dd869c5
c8b41b7
98917e9
9a91b1d
1dbb92f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
language: dart | ||
dart: | ||
- "1.17.1" | ||
- "1.19.1" | ||
with_content_shell: true | ||
before_install: | ||
- export DISPLAY=:99.0 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,14 @@ import './test_component_declaration.dart'; | |
|
||
/// Verify that treating the invocation of the builder as an unparameterized [ReactElement] | ||
/// does not result in the following analyzer warning: | ||
/// > Unsound implicit cast from ReactElement<dynamic> to ReactElement<Component> | ||
void _reactElementTypingTest() { | ||
/// | ||
/// Unsound implicit cast from ReactElement<dynamic> to ReactElement<Component> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
ReactElement _reactElementTypingTest() { | ||
ReactElement element = Foo()(); | ||
|
||
return element; | ||
} | ||
|
||
main() { | ||
_reactElementTypingTest(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,19 +1,18 @@ | ||
name: import_web_skin_dart_test | ||
name: import_over_react_test | ||
version: 0.0.1 | ||
description: Test importing web_skin_dart | ||
description: Test importing over_react | ||
environment: | ||
sdk: ">=1.12.1" | ||
sdk: ">=1.19.1" | ||
dependencies: | ||
browser: ">=0.10.0 <0.11.0" | ||
crypto: "0.9.2+1" | ||
source_gen: "^0.4.1" | ||
web_skin_dart: | ||
over_react: | ||
git: | ||
url: git://127.0.0.1:9000/ | ||
ref: HEAD | ||
dev_dependencies: | ||
dart_dev: "^1.0.5" | ||
test: "^0.12.6+2" | ||
transformers: | ||
- test/pub_serve: | ||
$include: test/test_runtime_import.dart | ||
- test/pub_serve: | ||
$include: test/test_runtime_import.dart |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,7 +48,8 @@ ReactDartComponentFactoryProxy registerComponent(react.Component dartComponentFa | |
Type componentClass, | ||
String displayName | ||
}) { | ||
ReactDartComponentFactoryProxy reactComponentFactory = react.registerComponent(dartComponentFactory); | ||
// ignore: avoid_as | ||
final reactComponentFactory = react.registerComponent(dartComponentFactory) as ReactDartComponentFactoryProxy; | ||
|
||
if (displayName != null) { | ||
reactComponentFactory.reactClass.displayName = displayName; | ||
|
@@ -84,8 +85,6 @@ typedef TProps UiFactory<TProps extends UiProps>([Map backingProps]); | |
/// For use as a Function variable type when the `backingProps` argument is not required. | ||
typedef TProps BuilderOnlyUiFactory<TProps extends UiProps>(); | ||
|
||
typedef dynamic _RefTypedef(String ref); | ||
|
||
/// The basis for a over_react component. | ||
/// | ||
/// Includes support for strongly-typed props and utilities for prop and CSS classname forwarding. | ||
|
@@ -94,14 +93,6 @@ typedef dynamic _RefTypedef(String ref); | |
/// | ||
/// Related: [UiStatefulComponent] | ||
abstract class UiComponent<TProps extends UiProps> extends react.Component { | ||
/// Returns the component of the specified [ref]. | ||
/// > `react.Component` if it is a Dart component | ||
/// > DOM node if it is a DOM component. | ||
/// | ||
/// Overridden for strong typing. | ||
@override | ||
_RefTypedef get ref => super.ref; | ||
|
||
/// The props for the non-forwarding props defined in this component. | ||
Iterable<ConsumedProps> get consumedProps => null; | ||
|
||
|
@@ -166,7 +157,7 @@ abstract class UiComponent<TProps extends UiProps> extends react.Component { | |
// BEGIN Typed props helpers | ||
// | ||
|
||
// Keep this Expando unparameterized to work around this bug: https://code.google.com/p/dart/issues/detail?id=18713 | ||
/// Keep this Expando unparameterized to work around this bug: <https://github.com/dart-lang/sdk/issues/26743> | ||
Expando _typedPropsCache = new Expando(); | ||
|
||
/// A typed props object corresponding to the current untyped props Map ([unwrappedProps]). | ||
|
@@ -175,7 +166,10 @@ abstract class UiComponent<TProps extends UiProps> extends react.Component { | |
@override | ||
TProps get props { | ||
var unwrappedProps = this.unwrappedProps; | ||
TProps typedProps = _typedPropsCache[unwrappedProps]; | ||
/// Have to cast as [TProps] until we can parameterize [_typedPropsCache]. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, I wonder if we could reintroduce the type parameter on this Expando to avoid this cast... it seems the linked issue might only apply to mixins, so the omitted parameterization may just be carried over from when this code was part of a mixin. These getters are used a lot, so it would be nice to avoid the perf hit of this cast if possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just tried this locally and it works There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @greglittlefield-wf @jacehensley-wf |
||
/// | ||
/// See: <https://github.com/dart-lang/sdk/issues/26743> | ||
var typedProps = _typedPropsCache[unwrappedProps] as TProps; // ignore: avoid_as | ||
if (typedProps == null) { | ||
typedProps = typedPropsFactory(unwrappedProps); | ||
_typedPropsCache[unwrappedProps] = typedProps; | ||
|
@@ -217,7 +211,7 @@ abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiStat | |
// BEGIN Typed state helpers | ||
// | ||
|
||
// Keep this Expando unparameterized to work around this bug: https://code.google.com/p/dart/issues/detail?id=18713 | ||
/// Keep this Expando unparameterized to work around this bug: <https://github.com/dart-lang/sdk/issues/26743> | ||
Expando _typedStateCache = new Expando(); | ||
|
||
/// A typed state object corresponding to the current untyped state Map ([unwrappedState]). | ||
|
@@ -226,7 +220,10 @@ abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiStat | |
@override | ||
TState get state { | ||
var unwrappedState = this.unwrappedState; | ||
TState typedState = _typedStateCache[unwrappedState]; | ||
/// Have to cast as [TState] until we can parameterize [_typedStateCache]. | ||
/// | ||
/// See: <https://github.com/dart-lang/sdk/issues/26743> | ||
var typedState = _typedStateCache[unwrappedState] as TState; // ignore: avoid_as | ||
if (typedState == null) { | ||
typedState = typedStateFactory(unwrappedState); | ||
_typedStateCache[unwrappedState] = typedState; | ||
|
@@ -387,7 +384,7 @@ abstract class UiProps | |
} | ||
|
||
/// Validates that no [children] are instances of [UiProps], and prints a helpful message for a better debugging | ||
/// experiance. | ||
/// experience. | ||
bool _validateChildren(dynamic children) { | ||
// Should not validate non-list iterables to avoid more than one iteration. | ||
if (children != null && (children is! Iterable || children is List)) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,8 +64,8 @@ class ParsedDeclarations { | |
|
||
// Validate the types of the annotated declarations. | ||
|
||
List topLevelVarsOnly(String annotationName, Iterable<CompilationUnitMember> declarations) { | ||
var topLevelVarDeclarations = []; | ||
List<CompilationUnitMember> topLevelVarsOnly(String annotationName, Iterable<CompilationUnitMember> declarations) { | ||
var topLevelVarDeclarations = <CompilationUnitMember>[]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these should be |
||
|
||
declarations.forEach((declaration) { | ||
if (declaration is TopLevelVariableDeclaration) { | ||
|
@@ -81,8 +81,8 @@ class ParsedDeclarations { | |
return topLevelVarDeclarations; | ||
}; | ||
|
||
List classesOnly(String annotationName, Iterable<CompilationUnitMember> declarations) { | ||
var classDeclarations = []; | ||
List<CompilationUnitMember> classesOnly(String annotationName, Iterable<CompilationUnitMember> declarations) { | ||
var classDeclarations = <CompilationUnitMember>[]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ..and these should be |
||
|
||
declarations.forEach((declaration) { | ||
if (declaration is ClassDeclaration) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two more spots where the analyzer is failing on 1.20.1, but they're easy fixes:
test/over_react/component_declaration/flux_component_test/handler_precedence.dart L21-25
Same thing for test/over_react/component_declaration/flux_component_test/store_handlers.dart L16-21.
With those two changes, I think you'd be fine to bump this up to 1.20.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if you're not using the variable is has to be typed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it has something to do with overriding the method but omitting the return type, I'm not sure.