Skip to content
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

Merged
merged 30 commits into from
Nov 2, 2016
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
27c1a59
First pass at strong mode compliance
aaronlademann-wf Oct 28, 2016
f2a5cc6
Get rid of two more implicit type casts
aaronlademann-wf Oct 28, 2016
f1c6657
Use https to see if travis likes that better
aaronlademann-wf Oct 28, 2016
17f16fa
Get rid of two implicit casts that don’t cause warnings on my machine
aaronlademann-wf Oct 28, 2016
2804edf
Cast in the test instead of changing the type of the method
aaronlademann-wf Oct 28, 2016
d9071f8
Change Function to ReactComponent
aaronlademann-wf Oct 28, 2016
af8d5f9
Remove redundant left hand side typing
aaronlademann-wf Oct 28, 2016
2914187
Turn on strong mode for integrate directory
aaronlademann-wf Oct 28, 2016
3b51626
Undo pubspec path commit
aaronlademann-wf Oct 28, 2016
0e7c44a
Fix implicit cast
aaronlademann-wf Oct 28, 2016
c2299be
Add test coverage for string UiComponent.ref
aaronlademann-wf Oct 28, 2016
fc1b96e
Use the typedef from react-dart
aaronlademann-wf Oct 31, 2016
b76c072
Don’t commit integrate/pubspec.lock
aaronlademann-wf Oct 31, 2016
fe69d03
Ignore all pubspec.lock files
aaronlademann-wf Oct 31, 2016
46639f2
Bump min sdk to 1.19.1
aaronlademann-wf Oct 31, 2016
047a8a9
Merge 'aaronlademann-wf/sdk_1.19.1' into strong-mode-ftw
aaronlademann-wf Oct 31, 2016
9d6a136
Remove unnecessary casts
aaronlademann-wf Oct 31, 2016
0fe7556
Parameterize Lists / Maps for 1.20.1 strong mode compliance
aaronlademann-wf Nov 1, 2016
2507729
Remove unnecessary casting
aaronlademann-wf Nov 1, 2016
1b4a80a
Enable avoid_as lint
aaronlademann-wf Nov 1, 2016
d205ea9
Add casting to fix broken VM tests
aaronlademann-wf Nov 1, 2016
5547f8d
Fix the vm test errors a better way
aaronlademann-wf Nov 1, 2016
c4dd932
Fix analysis error when sdk version is > 1.20.1
aaronlademann-wf Nov 1, 2016
f211b71
Update react dependency to ^3.0.1
aaronlademann-wf Nov 1, 2016
ba742e8
Remove unnecessary test file
aaronlademann-wf Nov 1, 2016
dd869c5
Address CR feedback
aaronlademann-wf Nov 1, 2016
c8b41b7
Parameterize Expando
aaronlademann-wf Nov 1, 2016
98917e9
Fix some formatting anomalies
aaronlademann-wf Nov 1, 2016
9a91b1d
Exclude integrate/ directory from analyzer
aaronlademann-wf Nov 1, 2016
1dbb92f
Address CR feedback
aaronlademann-wf Nov 1, 2016
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .analysis_options
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
analyzer:
strong-mode: true
exclude:
- integrate/**
linter:
Expand Down
8 changes: 5 additions & 3 deletions lib/src/component/prop_mixins.dart
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,14 @@ abstract class DomPropsMixin {

Map<String, dynamic> style;

String className, title, id;

dynamic accept, acceptCharset, accessKey, action, allowTransparency, alt, autoComplete, cellPadding, cellSpacing,
charSet, classID, className, colSpan, content, contentEditable, contextMenu, coords, crossOrigin, data, dateTime,
dir, download, draggable, encType, form, frameBorder, height, href, hrefLang, htmlFor, httpEquiv, icon, id, label,
charSet, classID, colSpan, content, contentEditable, contextMenu, coords, crossOrigin, data, dateTime,
dir, download, draggable, encType, form, frameBorder, height, href, hrefLang, htmlFor, httpEquiv, icon, label,
lang, list, manifest, max, maxLength, media, mediaGroup, method, min, name, open, pattern, placeholder,
poster, preload, radioGroup, rel, role, rowSpan, sandbox, scope, scrolling, shape, sizes, spellCheck, src, srcDoc,
srcSet, step, tabIndex, target, title, type, useMap, value, width, wmode;
srcSet, step, tabIndex, target, type, useMap, value, width, wmode;

ClipboardEventCallback onCopy, onCut, onPaste;
KeyboardEventCallback onKeyDown, onKeyPress, onKeyUp;
Expand Down
2 changes: 1 addition & 1 deletion lib/src/component/resize_sensor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ class ResizeSensorComponent extends UiComponent<ResizeSensorProps> {
)(expandSensor, collapseSensor)
);

var wrapperStyles;
Map<String, dynamic> wrapperStyles;

if (props.isFlexChild) {
wrapperStyles = {
Expand Down
21 changes: 12 additions & 9 deletions lib/src/component_declaration/component_base.dart
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ ReactDartComponentFactoryProxy registerComponent(react.Component dartComponentFa
Type componentClass,
String displayName
}) {
ReactDartComponentFactoryProxy reactComponentFactory = react.registerComponent(dartComponentFactory);
ReactDartComponentFactoryProxy reactComponentFactory =
(react.registerComponent(dartComponentFactory)) as ReactDartComponentFactoryProxy;

if (displayName != null) {
reactComponentFactory.reactClass.displayName = displayName;
Expand Down Expand Up @@ -94,27 +95,29 @@ typedef dynamic _RefTypedef(String ref);
///
/// Related: [UiStatefulComponent]
abstract class UiComponent<TProps extends UiProps> extends react.Component {
/// The props for the non-forwarding props defined in this component.
Iterable<ConsumedProps> get consumedProps => null;
/// 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;
_RefTypedef get ref => super.ref as _RefTypedef;

/// The props for the non-forwarding props defined in this component.
Iterable<ConsumedProps> get consumedProps => null;

/// Returns a copy of this component's props with [consumedPropKeys] omitted.
Map copyUnconsumedProps() {
var consumedPropKeys = consumedProps?.map((ConsumedProps consumedProps) => consumedProps.keys) ?? const [];
var consumedPropKeys =
(consumedProps?.map((ConsumedProps consumedProps) => consumedProps.keys) ?? const []) as Iterable<Iterable>;

return copyProps(keySetsToOmit: consumedPropKeys);
}

/// Returns a copy of this component's props with [consumedPropKeys] and non-DOM props omitted.
Map copyUnconsumedDomProps() {
var consumedPropKeys = consumedProps?.map((ConsumedProps consumedProps) => consumedProps.keys) ?? const [];
var consumedPropKeys =
(consumedProps?.map((ConsumedProps consumedProps) => consumedProps.keys) ?? const []) as Iterable<Iterable>;

return copyProps(onlyCopyDomProps: true, keySetsToOmit: consumedPropKeys);
}
Expand Down Expand Up @@ -175,7 +178,7 @@ abstract class UiComponent<TProps extends UiProps> extends react.Component {
@override
TProps get props {
var unwrappedProps = this.unwrappedProps;
TProps typedProps = _typedPropsCache[unwrappedProps];
TProps typedProps = _typedPropsCache[unwrappedProps] as TProps;
if (typedProps == null) {
typedProps = typedPropsFactory(unwrappedProps);
_typedPropsCache[unwrappedProps] = typedProps;
Expand Down Expand Up @@ -226,7 +229,7 @@ abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiStat
@override
TState get state {
var unwrappedState = this.unwrappedState;
TState typedState = _typedStateCache[unwrappedState];
TState typedState = _typedStateCache[unwrappedState] as TState;
if (typedState == null) {
typedState = typedStateFactory(unwrappedState);
_typedStateCache[unwrappedState] = typedState;
Expand Down Expand Up @@ -387,7 +390,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)) {
Expand Down
11 changes: 5 additions & 6 deletions lib/src/component_declaration/flux_component.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ abstract class FluxUiProps<ActionsT, StoresT> extends UiProps {
/// There is no strict rule on the [ActionsT] type. Depending on application
/// structure, there may be [Action]s available directly on this object, or
/// this object may represent a hierarchy of actions.
ActionsT get actions => props[_actionsPropKey];
ActionsT get actions => props[_actionsPropKey] as ActionsT;
set actions(ActionsT value) => props[_actionsPropKey] = value;

/// The prop defined by [StoresT].
Expand All @@ -37,7 +37,7 @@ abstract class FluxUiProps<ActionsT, StoresT> extends UiProps {
/// [StoresT] should be a class that provides access to these multiple stores.
/// Then, you can explicitly select the [Store] instances that should be
/// listened to by overriding [_FluxComponentMixin.redrawOn].
StoresT get store => props[_storePropKey];
StoresT get store => props[_storePropKey] as StoresT;
set store(StoresT value) => props[_storePropKey] = value;
}

Expand Down Expand Up @@ -67,10 +67,8 @@ abstract class FluxUiStatefulComponent<TProps extends FluxUiProps, TState extend
/// Helper mixin to keep [FluxUiComponent] and [FluxUiStatefulComponent] clean/DRY.
///
/// Private so it will only get used in this file, since having lifecycle methods in a mixin is risky.
abstract class _FluxComponentMixin<TProps extends FluxUiProps> {
abstract class _FluxComponentMixin<TProps extends FluxUiProps> implements BatchedRedraws {
TProps get props;
bool shouldBatchRedraw;
redraw([callback()]);

/// List of store subscriptions created when the component mounts.
///
Expand All @@ -87,8 +85,9 @@ abstract class _FluxComponentMixin<TProps extends FluxUiProps> {
/// respective handlers.
Map<Store, Function> handlers = new Map.fromIterable(redrawOn(),
value: (_) => (_) => redraw())..addAll(getStoreHandlers());

handlers.forEach((store, handler) {
StreamSubscription subscription = store.listen(handler);
StreamSubscription subscription = store.listen(handler as StoreHandler);
_subscriptions.add(subscription);
});
}
Expand Down
22 changes: 0 additions & 22 deletions lib/src/component_declaration/transformer_helpers.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,13 @@

library over_react.component_declaration.transformer_helpers;

import 'dart:html';

import './component_base.dart' as component_base;
import './annotations.dart' as annotations;

export './annotations.dart';
export './component_base.dart'
hide UiComponent, UiStatefulComponent, UiProps, UiState;

typedef dynamic _RefTypedef(String ref);

// ----------------------------------------------------------------------
// Helpers and extras consumable by generated code and consumers of
// generated code.
Expand Down Expand Up @@ -99,15 +95,6 @@ abstract class UiComponent<TProps extends UiProps> extends component_base.UiComp
_throwIfNotGenerated();
}

/// Returns the component of the specified [ref].
///
/// * Will be a [react.Component] if it is a Dart component.
/// * [Element] if it is a Dom component.
///
/// Overridden for strong typing.
@override
_RefTypedef get ref => super.ref;

/// The default consumed props, taken from the keys generated in the associated @[annotations.Props] class.
@toBeGenerated
Iterable<component_base.ConsumedProps> get $defaultConsumedProps => throw new UngeneratedError(member: #$defaultConsumedProps);
Expand Down Expand Up @@ -137,15 +124,6 @@ abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiStat
_throwIfNotGenerated();
}

/// Returns the component of the specified [ref].
///
/// * Will be a [react.Component] if it is a Dart component.
/// * [Element] if it is a Dom component.
///
/// Overridden for strong typing.
@override
_RefTypedef get ref => super.ref;

/// The default consumed prop keys, taken from the keys generated in the associated @[annotations.Props] class.
@toBeGenerated
Iterable<component_base.ConsumedProps> get $defaultConsumedProps => throw new UngeneratedError(member: #$defaultConsumedProps);
Expand Down
6 changes: 4 additions & 2 deletions lib/src/transformer/declaration_parsing.dart
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ class ParsedDeclarations {
return classDeclarations;
};

declarationMap[key_factory] = topLevelVarsOnly(key_factory, declarationMap[key_factory]);
declarationMap[key_factory] =
(topLevelVarsOnly(key_factory, declarationMap[key_factory])) as Iterable<CompilationUnitMember>;

[
key_component,
Expand All @@ -110,7 +111,8 @@ class ParsedDeclarations {
key_propsMixin,
key_stateMixin,
].forEach((annotationName) {
declarationMap[annotationName] = classesOnly(annotationName, declarationMap[annotationName]);
declarationMap[annotationName] =
(classesOnly(annotationName, declarationMap[annotationName])) as Iterable<CompilationUnitMember>;
});


Expand Down
23 changes: 12 additions & 11 deletions lib/src/transformer/impl_generation.dart
Original file line number Diff line number Diff line change
Expand Up @@ -364,19 +364,20 @@ class ImplGenerator {

typedMap.node.members
.where((member) => member is FieldDeclaration)
.where((FieldDeclaration member) => !member.isStatic)
.forEach((FieldDeclaration field) {
.where((member) => !(member as FieldDeclaration).isStatic)
.forEach((field) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#nit Just so there is not a bunch of changes could the param be prefixed with an underscore and the new var not?

.forEach((_field) {
  final field = _field as FieldDeclaration;
})

FieldDeclaration _field = (field as FieldDeclaration);
// Remove everything in the field except the comments/meta and the variable names, preserving newlines.
// TODO add support for preserving comment nodes between variable declarations.

// Remove content between end of comment/meta and first variable name
transformedFile.remove(
sourceFile.span(field.firstTokenAfterCommentAndMetadata.offset, field.fields.variables.first.beginToken.offset),
sourceFile.span(_field.firstTokenAfterCommentAndMetadata.offset, _field.fields.variables.first.beginToken.offset),
preserveNewlines: true
);
// Remove content between variable names (including commas).
var prevVariable = field.fields.variables.first;
field.fields.variables.skip(1).forEach((variable) {
var prevVariable = _field.fields.variables.first;
_field.fields.variables.skip(1).forEach((variable) {
transformedFile.remove(
sourceFile.span(prevVariable.name.end, variable.name.offset),
preserveNewlines: true
Expand All @@ -385,11 +386,11 @@ class ImplGenerator {
});
// Remove content between last variable name and the end of the field (including the semicolon).
transformedFile.remove(
sourceFile.span(field.fields.variables.last.end, field.end),
sourceFile.span(_field.fields.variables.last.end, field.end),
preserveNewlines: true
);

field.fields.variables.forEach((VariableDeclaration variable) {
_field.fields.variables.forEach((VariableDeclaration variable) {
if (variable.initializer != null) {
logger.error(
'Fields are stubs for generated setters/getters and should not have initializers.',
Expand Down Expand Up @@ -427,7 +428,7 @@ class ImplGenerator {
keyConstants[keyConstantName] = keyValue;
constants[constantName] = constantValue;

TypeName type = field.fields.type;
TypeName type = _field.fields.type;
String typeString = type == null ? '' : '$type ';

String generatedAccessor =
Expand All @@ -444,12 +445,12 @@ class ImplGenerator {
);
});

if (field.fields.variables.length > 1 &&
(field.documentationComment != null || field.metadata.isNotEmpty)) {
if (_field.fields.variables.length > 1 &&
(_field.documentationComment != null || _field.metadata.isNotEmpty)) {
logger.warning(
'Note: accessors declared as comma-separated variables will not all be generated '
'with the original doc comments and annotations; only the first variable will.',
span: getSpan(sourceFile, field.fields)
span: getSpan(sourceFile, _field.fields)
);
}
});
Expand Down
3 changes: 2 additions & 1 deletion lib/src/util/class_names.dart
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ import 'dart:collection';
import 'package:over_react/over_react.dart' show PropDescriptor, ConsumedProps;
import 'package:over_react/src/component_declaration/annotations.dart';

/// Typed getters/setters for props related to CSS class manipulation, and used by all UIP components.
/// Typed getters/setters for props related to CSS class manipulation, and used by all over_react components.
///
/// To be used as a mixin for React components and builders.
@PropsMixin(keyNamespace: '')
abstract class CssClassPropsMixin {
Expand Down
6 changes: 6 additions & 0 deletions pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ dev_dependencies:
mockito: "^0.11.0"
test: "^0.12.6+2"

dependency_overrides:
react:
git:
url: https://github.com/aaronlademann-wf/react-dart.git
ref: over_react-strong-mode-tweaks

transformers:
- over_react:
$exclude: [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ main() {

group('modifyProps()', () {
test('passes the provided modifier itself', () {
modifier(UiProps props) {
modifier(Map<dynamic, dynamic> props) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#nit Unnecessary parameters

props['className'] = 'modified-class-name';
}

Expand Down Expand Up @@ -702,7 +702,7 @@ class TestComponentComponent extends UiComponent<TestComponentProps> {
@override
final List<ConsumedProps> consumedProps;

TestComponentComponent({testConsumedProps}) : consumedProps = testConsumedProps;
TestComponentComponent({List<ConsumedProps> testConsumedProps}) : consumedProps = testConsumedProps;

@override
render() => false;
Expand Down
13 changes: 7 additions & 6 deletions test/over_react/util/handler_chain_util_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ main() {
}

List<TestGenericType> generateBadTypeArgs() {
return new List.generate(arity, (_) => new Object());
// ignore: Type check failed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think this should just be List generateBadTypeArgs() {

return new List.generate(arity, (_) => new Object() as TestGenericType);
}

group('chain()', () {
Expand Down Expand Up @@ -158,7 +159,7 @@ main() {
test('calls all functions in order', () {
var calls = [];

var functions = new List.generate(5, (index) {
List<Function> functions = new List.generate(5, (index) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#supernit for something like this, I think it might be preferred to do

var functions = new List<Function>.generate(5, (index) {

@evanweible-wf ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greglittlefield-wf @evanweible-wf when I do that, I get the following test failures:

00:11 +887 -1: [Dartium Content Shell] test/over_react_test.dart: HandlerChainUtil generic chaining: CallbackUtil0Arg chainFromList() returns a function of arity 0 that calls all functions in order
      type 'List<Function>' is not a subtype of type 'List<Callback0Arg>' of 'callbacks' where
        List is from dart:core
        Function is from dart:core
        List is from dart:core

      package:over_react/src/util/handler_chain_util.dart 212:41  CallbackUtil.chainFromList
      over_react/util/handler_chain_util_test.dart 167:42         main.<fn>.<fn>.sharedTests.<fn>.<fn>.<fn>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry, should it be this, then?

var functions = new List<Callback0Arg>.generate(5, (index) {

I'm fine leaving it as-is if it works, though

return createTestChainFunction(onCall: (args) {
calls.add(['function_$index', args]);
});
Expand All @@ -180,7 +181,7 @@ main() {
});

test('returns false when any function returns false', () {
var functions = new List.generate(5, (_) => createTestChainFunction());
List<Function> functions = new List.generate(5, (_) => createTestChainFunction());
functions.insert(2, createTestChainFunction(returnValue: false));

var chained = callbackUtil.chainFromList(functions);
Expand All @@ -189,7 +190,7 @@ main() {
});

test('returns null when no function returns false', () {
var functions = new List.generate(5, (_) => createTestChainFunction());
List<Function> functions = new List.generate(5, (_) => createTestChainFunction());

var chained = callbackUtil.chainFromList(functions);

Expand All @@ -201,7 +202,7 @@ main() {
test('null functions', () {
var calls = [];

var functions = new List.generate(5, (index) {
List<Function> functions = new List.generate(5, (index) {
return createTestChainFunction(onCall: (args) {
calls.add(['function_$index', args]);
});
Expand Down Expand Up @@ -236,7 +237,7 @@ main() {

if (arity != 0) {
test('has arguments typed to the specified generic parameters', () {
var functions = new List.generate(5, (_) => createTestChainFunction());
List<Function> functions = new List.generate(5, (_) => createTestChainFunction());

functions.forEach((function) {
expect(() => Function.apply(function, generateArgs()), returnsNormally,
Expand Down
6 changes: 4 additions & 2 deletions test/over_react/util/react_wrappers_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -457,7 +457,7 @@ main() {

group('getProps', () {
const List testChildren = const ['child1', 'child2'];
const Map testStyle = const {'background': 'white'};
const Map<String, dynamic> testStyle = const {'background': 'white'};

test('returns props for a composite JS component ReactElement', () {
ReactElement instance = render(testJsComponentFactory({
Expand Down Expand Up @@ -687,7 +687,9 @@ main() {
}

/// Helper component for testing a Dart (react-dart) React component with cloneElement.
ReactComponentFactory TestComponentFactory = react.registerComponent(() => new TestComponent());
ReactComponentFactory TestComponentFactory =
(react.registerComponent(() => new TestComponent())) as ReactComponentFactory;

class TestComponent extends react.Component {
@override
render() => Dom.div()();
Expand Down
Loading