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 29 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
2 changes: 2 additions & 0 deletions .analysis_options
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
analyzer:
exclude:
- integrate/**
strong-mode: true
linter:
rules:
- annotate_overrides
- avoid_as
- avoid_empty_else
- avoid_init_to_null
- avoid_return_types_on_setters
Expand Down
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ build/

# Since over_react is a library package, we shouldn't track pubspec.lock:
# https://www.dartlang.org/tools/pub/glossary.html#library-package
/pubspec.lock
pubspec.lock

# Should not track the output of code coverage for now
coverage/
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
language: dart
dart:
- "1.17.1"
- "1.19.1"
Copy link
Contributor

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

  @override
  getStoreHandlers() => {props.store.store1: increment};

- increment(_) {
+ increment(Store store) {
    numberOfHandlerCalls += 1;
  }

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

Copy link
Contributor

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?

Copy link
Contributor

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.

with_content_shell: true
before_install:
- export DISPLAY=:99.0
Expand Down
5 changes: 2 additions & 3 deletions integrate/lib/test_component_declaration.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@

library test_component_declaration;

import 'package:over_react/ui_core.dart';
import 'package:over_react/ui_components.dart';
import 'package:over_react/over_react.dart';

@Factory()
UiFactory<FooProps> Foo;
Expand All @@ -30,7 +29,7 @@ class FooState extends UiState {}
class FooComponent extends UiStatefulComponent<FooProps, FooState> {
@override
render() {
return Button()(
return Dom.div()(
'Child 1',
'Child 2'
);
Expand Down
26 changes: 0 additions & 26 deletions integrate/lib/test_react_element_typing.dart

This file was deleted.

13 changes: 6 additions & 7 deletions integrate/pubspec.yaml
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
6 changes: 2 additions & 4 deletions integrate/test/test_runtime_import.dart
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,7 @@ library test;
import 'package:react/react_client.dart' show setClientConfiguration;
import 'package:react/react_test_utils.dart' as react_test_utils;
import 'package:test/test.dart';
import 'package:over_react/test_util.dart';
import 'package:over_react/ui_components.dart';
import 'package:over_react/ui_core.dart';
import 'package:over_react/over_react.dart';

/// Entry point into tests
main() {
Expand All @@ -34,7 +32,7 @@ main() {
});

test('renders without breaking', () {
var well = react_test_utils.renderIntoDocument(Well()('Hello World!'));
var well = react_test_utils.renderIntoDocument(Dom.div()('Hello World!'));
expect(well, isNotNull);
});
}
4 changes: 2 additions & 2 deletions integrate/web/test_import.dart
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ import 'dart:html';
import 'package:react/react_dom.dart' as react_dom;
import 'package:react/react_client.dart' show setClientConfiguration;

import 'package:over_react/ui_components.dart';
import 'package:over_react/over_react.dart';

void main() {
setClientConfiguration();

react_dom.render(Well()('Hello World!'), querySelector('.main'));
react_dom.render(Dom.div()('Hello World!'), querySelector('.main'));
}
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
37 changes: 14 additions & 23 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);
// ignore: avoid_as
final reactComponentFactory = react.registerComponent(dartComponentFactory) as ReactDartComponentFactoryProxy;

if (displayName != null) {
reactComponentFactory.reactClass.displayName = displayName;
Expand Down Expand Up @@ -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.
Expand All @@ -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;

Expand Down Expand Up @@ -133,12 +124,12 @@ abstract class UiComponent<TProps extends UiProps> extends react.Component {
void validateRequiredProps(Map appliedProps) {
consumedProps?.forEach((ConsumedProps consumedProps) {
consumedProps.props.forEach((PropDescriptor prop) {
if (!prop.isRequired) return;
if (prop.isNullable && appliedProps.containsKey(prop.key)) return;
if (!prop.isNullable && appliedProps[prop.key] != null) return;
if (!prop.isRequired) return;
if (prop.isNullable && appliedProps.containsKey(prop.key)) return;
if (!prop.isNullable && appliedProps[prop.key] != null) return;

throw new PropError.required(prop.key, prop.errorMessage);
});
throw new PropError.required(prop.key, prop.errorMessage);
});
});
}

Expand Down Expand Up @@ -166,16 +157,15 @@ 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
Expando _typedPropsCache = new Expando();
var _typedPropsCache = new Expando<TProps>();

/// A typed props object corresponding to the current untyped props Map ([unwrappedProps]).
///
/// Created using [typedPropsFactory] and cached for each Map instance.
@override
TProps get props {
var unwrappedProps = this.unwrappedProps;
TProps typedProps = _typedPropsCache[unwrappedProps];
var typedProps = _typedPropsCache[unwrappedProps];
if (typedProps == null) {
typedProps = typedPropsFactory(unwrappedProps);
_typedPropsCache[unwrappedProps] = typedProps;
Expand All @@ -191,6 +181,7 @@ abstract class UiComponent<TProps extends UiProps> extends react.Component {
set unwrappedProps(Map value) => super.props = value;

/// Returns a typed props object backed by the specified [propsMap].
///
/// Required to properly instantiate the generic [TProps] class.
TProps typedPropsFactory(Map propsMap);

Expand All @@ -217,16 +208,15 @@ 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
Expando _typedStateCache = new Expando();
var _typedStateCache = new Expando<TState>();

/// A typed state object corresponding to the current untyped state Map ([unwrappedState]).
///
/// Created using [typedStateFactory] and cached for each Map instance.
@override
TState get state {
var unwrappedState = this.unwrappedState;
TState typedState = _typedStateCache[unwrappedState];
var typedState = _typedStateCache[unwrappedState];
if (typedState == null) {
typedState = typedStateFactory(unwrappedState);
_typedStateCache[unwrappedState] = typedState;
Expand All @@ -242,6 +232,7 @@ abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiStat
set unwrappedState(Map value) => super.state = value;

/// Returns a typed state object backed by the specified [stateMap].
///
/// Required to properly instantiate the generic [TState] class.
TState typedStateFactory(Map stateMap);

Expand Down Expand Up @@ -387,7 +378,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
15 changes: 7 additions & 8 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; // ignore: avoid_as
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; // ignore: avoid_as
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 @@ -85,8 +83,9 @@ abstract class _FluxComponentMixin<TProps extends FluxUiProps> {
///
/// [Store]s included in the [getStoreHandlers] result will be listened to and wired up to their
/// respective handlers.
Map<Store, Function> handlers = new Map.fromIterable(redrawOn(),
Map<Store, StoreHandler> handlers = new Map.fromIterable(redrawOn(),
value: (_) => (_) => redraw())..addAll(getStoreHandlers());

handlers.forEach((store, handler) {
StreamSubscription subscription = store.listen(handler);
_subscriptions.add(subscription);
Expand Down Expand Up @@ -122,7 +121,7 @@ abstract class _FluxComponentMixin<TProps extends FluxUiProps> {
/// redrawOn() => [store.tasks, store.users];
List<Store> redrawOn() {
if (props.store is Store) {
return [props.store];
return <Store>[props.store];
} else {
return [];
}
Expand All @@ -137,7 +136,7 @@ abstract class _FluxComponentMixin<TProps extends FluxUiProps> {
/// If possible, however, [redrawOn] should be used instead of this in order
/// to avoid keeping additional state within this component and manually
/// managing redraws.
Map<Store, Function> getStoreHandlers() {
Map<Store, StoreHandler> getStoreHandlers() {
return {};
}

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
Loading