From f41c9f3c496aa181f0bb1ae7d5e6c57e1fb3b31b Mon Sep 17 00:00:00 2001 From: Joe Bingham Date: Fri, 15 Feb 2019 14:20:54 -0700 Subject: [PATCH 01/15] Add a class watching for direct props/state mutation --- .../component_declaration/component_base.dart | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/src/component_declaration/component_base.dart b/lib/src/component_declaration/component_base.dart index 4bfb2c998..ba0734604 100644 --- a/lib/src/component_declaration/component_base.dart +++ b/lib/src/component_declaration/component_base.dart @@ -245,7 +245,7 @@ abstract class UiComponent extends react.Component imple var unwrappedProps = this.unwrappedProps; var typedProps = _typedPropsCache[unwrappedProps]; if (typedProps == null) { - typedProps = typedPropsFactory(unwrappedProps); + typedProps = typedPropsFactory(WarnOnModify(unwrappedProps)); _typedPropsCache[unwrappedProps] = typedProps; } return typedProps; @@ -415,7 +415,7 @@ abstract class UiStatefulComponent extends MapView { + WarnOnModify(Map componentData): super(componentData); + + @override + operator []=(K key, V value) { + assert( + super[key] != value, + 'Mutating the state or props directly will soon be impossible without causing breakages.' + ); + super[key] = value; + } +} /// A `dart.collection.MapView`-like class with strongly-typed getters/setters for React state. /// From 3caa5ddf30b1a6a2fedcdc76ca31bb4c1b223155 Mon Sep 17 00:00:00 2001 From: Joe Bingham Date: Fri, 15 Feb 2019 14:27:13 -0700 Subject: [PATCH 02/15] Change formatting --- lib/src/component_declaration/component_base.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/src/component_declaration/component_base.dart b/lib/src/component_declaration/component_base.dart index ba0734604..e85bb6c8e 100644 --- a/lib/src/component_declaration/component_base.dart +++ b/lib/src/component_declaration/component_base.dart @@ -446,7 +446,7 @@ abstract class UiStatefulComponent extends MapView { WarnOnModify(Map componentData): super(componentData); - + @override operator []=(K key, V value) { assert( From b01e4c8eac3ff534322b2e7bf2a5b49b654d8b01 Mon Sep 17 00:00:00 2001 From: Joe Bingham Date: Mon, 18 Feb 2019 13:45:10 -0700 Subject: [PATCH 03/15] Change the Assert condition --- lib/src/component_declaration/component_base.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/src/component_declaration/component_base.dart b/lib/src/component_declaration/component_base.dart index e85bb6c8e..52ef5dd54 100644 --- a/lib/src/component_declaration/component_base.dart +++ b/lib/src/component_declaration/component_base.dart @@ -450,7 +450,7 @@ class WarnOnModify extends MapView { @override operator []=(K key, V value) { assert( - super[key] != value, + false, 'Mutating the state or props directly will soon be impossible without causing breakages.' ); super[key] = value; From 95e6b7295458becf523ef8f86010adbd3ebfabf8 Mon Sep 17 00:00:00 2001 From: Joe Bingham Date: Mon, 18 Feb 2019 13:46:17 -0700 Subject: [PATCH 04/15] Add tests to check if the assert is thrown --- .../component_base_test.dart | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/test/over_react/component_declaration/component_base_test.dart b/test/over_react/component_declaration/component_base_test.dart index 6f1282a9e..b194680ee 100644 --- a/test/over_react/component_declaration/component_base_test.dart +++ b/test/over_react/component_declaration/component_base_test.dart @@ -124,6 +124,15 @@ main() { _commonNonInvokedBuilderTests(Dom.div()); }, testOn: '!js'); + test('warns against setting props directly', () { + + var instance = render(TestComponent()()); + var component = getDartComponent(instance); + var changeProps = () => component.props['id'] = 'test'; + + expect(changeProps, throwsA(const TypeMatcher())); + }); + group('renders a DOM component with the correct children when', () { _commonVariadicChildrenTests(Dom.div()); @@ -800,7 +809,7 @@ main() { setUp(() { statefulComponent = new TestStatefulComponentComponent(); - statefulComponent.unwrappedState = {}; + statefulComponent.unwrappedState = {'test': true}; }); group('`state`', () { @@ -827,6 +836,12 @@ main() { expect(stateBeforeChange, isNot(same(stateAfterChange))); }); + + test('warns against setting state directly', () { + var changeState = () => statefulComponent.state['test'] = true; + + expect(changeState, throwsA(const TypeMatcher())); + }); }); group('setter:', () { From f459e907261431de8c06954db4f076a7fd7288a6 Mon Sep 17 00:00:00 2001 From: Joe Bingham Date: Mon, 18 Feb 2019 13:53:50 -0700 Subject: [PATCH 05/15] Fix formatting --- test/over_react/component_declaration/component_base_test.dart | 1 - 1 file changed, 1 deletion(-) diff --git a/test/over_react/component_declaration/component_base_test.dart b/test/over_react/component_declaration/component_base_test.dart index b194680ee..caaa4831d 100644 --- a/test/over_react/component_declaration/component_base_test.dart +++ b/test/over_react/component_declaration/component_base_test.dart @@ -125,7 +125,6 @@ main() { }, testOn: '!js'); test('warns against setting props directly', () { - var instance = render(TestComponent()()); var component = getDartComponent(instance); var changeProps = () => component.props['id'] = 'test'; From c1237c62da53fac2557d55c4de31ced78d48ead2 Mon Sep 17 00:00:00 2001 From: Joe Bingham Date: Mon, 18 Feb 2019 14:20:58 -0700 Subject: [PATCH 06/15] Update testing to pass CI tests --- .../component_declaration/component_base_test.dart | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/test/over_react/component_declaration/component_base_test.dart b/test/over_react/component_declaration/component_base_test.dart index caaa4831d..976d53045 100644 --- a/test/over_react/component_declaration/component_base_test.dart +++ b/test/over_react/component_declaration/component_base_test.dart @@ -127,9 +127,10 @@ main() { test('warns against setting props directly', () { var instance = render(TestComponent()()); var component = getDartComponent(instance); - var changeProps = () => component.props['id'] = 'test'; - expect(changeProps, throwsA(const TypeMatcher())); + expect(() { + component.props['id'] = 'test'; + }, throwsA(const TypeMatcher())); }); group('renders a DOM component with the correct children when', () { @@ -837,9 +838,10 @@ main() { }); test('warns against setting state directly', () { - var changeState = () => statefulComponent.state['test'] = true; - - expect(changeState, throwsA(const TypeMatcher())); + + expect(() { + statefulComponent.state['test'] = true; + }, throwsA(const TypeMatcher())); }); }); From 84c1e400b2493060fbda3bc97d6621179cc1b24c Mon Sep 17 00:00:00 2001 From: Joe Bingham Date: Mon, 18 Feb 2019 14:46:01 -0700 Subject: [PATCH 07/15] Exclude .js from dev only tests --- .../component_declaration/component_base_test.dart | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/over_react/component_declaration/component_base_test.dart b/test/over_react/component_declaration/component_base_test.dart index 976d53045..25c507f19 100644 --- a/test/over_react/component_declaration/component_base_test.dart +++ b/test/over_react/component_declaration/component_base_test.dart @@ -131,7 +131,7 @@ main() { expect(() { component.props['id'] = 'test'; }, throwsA(const TypeMatcher())); - }); + }, testOn: '!js'); group('renders a DOM component with the correct children when', () { _commonVariadicChildrenTests(Dom.div()); @@ -838,11 +838,11 @@ main() { }); test('warns against setting state directly', () { - + expect(() { statefulComponent.state['test'] = true; }, throwsA(const TypeMatcher())); - }); + }, testOn: '!js'); }); group('setter:', () { From c15e0b74b3624ff38040ef28d9972980c1435383 Mon Sep 17 00:00:00 2001 From: Joe Bingham Date: Mon, 18 Feb 2019 15:21:37 -0700 Subject: [PATCH 08/15] Remove extra line --- test/over_react/component_declaration/component_base_test.dart | 1 - 1 file changed, 1 deletion(-) diff --git a/test/over_react/component_declaration/component_base_test.dart b/test/over_react/component_declaration/component_base_test.dart index 25c507f19..fff941733 100644 --- a/test/over_react/component_declaration/component_base_test.dart +++ b/test/over_react/component_declaration/component_base_test.dart @@ -838,7 +838,6 @@ main() { }); test('warns against setting state directly', () { - expect(() { statefulComponent.state['test'] = true; }, throwsA(const TypeMatcher())); From c0dc513d0729c2a0aae6f7f66311a7fce75ffe3a Mon Sep 17 00:00:00 2001 From: Joe Bingham Date: Tue, 19 Feb 2019 14:36:45 -0700 Subject: [PATCH 09/15] Update message and remove assert --- .../component_declaration/component_base.dart | 37 +++++++++++++++---- 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/lib/src/component_declaration/component_base.dart b/lib/src/component_declaration/component_base.dart index 52ef5dd54..96b71956c 100644 --- a/lib/src/component_declaration/component_base.dart +++ b/lib/src/component_declaration/component_base.dart @@ -16,6 +16,7 @@ library over_react.component_declaration.component_base; import 'dart:async'; import 'dart:collection'; +import 'dart:html'; import 'package:meta/meta.dart'; import 'package:over_react/over_react.dart'; @@ -245,7 +246,7 @@ abstract class UiComponent extends react.Component imple var unwrappedProps = this.unwrappedProps; var typedProps = _typedPropsCache[unwrappedProps]; if (typedProps == null) { - typedProps = typedPropsFactory(WarnOnModify(unwrappedProps)); + typedProps = typedPropsFactory(inReactDevMode ? _WarnOnModify(unwrappedProps, true) : unwrappedProps); _typedPropsCache[unwrappedProps] = typedProps; } return typedProps; @@ -415,7 +416,7 @@ abstract class UiStatefulComponent extends MapView { - WarnOnModify(Map componentData): super(componentData); +class _WarnOnModify extends MapView { + + //Used to customize warning based on whether the data is props or state + bool isProps; + + _WarnOnModify(Map componentData, bool isProps): super(componentData){ + this.isProps = isProps; + } @override operator []=(K key, V value) { - assert( - false, - 'Mutating the state or props directly will soon be impossible without causing breakages.' - ); + if (isProps) { + window.console.warn(unindent( + ''' + props["$key"] was updated incorrectly. Never mutate this.props directly, as it can cause unexpected behavior; props must be updated only by passing in new values when rerendering this component. + + This will throw in UiComponentV2 (to be released as part of the React 16 upgrade). + ''' + )); + } else { + window.console.warn(unindent( + ''' + state["$key"] was updated incorrectly. Never this.state directly, as it can cause unexpected behavior; state must be updated only via setState. + + This will throw in UiComponentV2 (to be released as part of the React 16 upgrade). + ''' + )); + } + super[key] = value; } } From 006340bdb786adee2343d38bd0de39b9d7d6707b Mon Sep 17 00:00:00 2001 From: Joe Bingham Date: Wed, 20 Feb 2019 09:23:29 -0700 Subject: [PATCH 10/15] Update tests to use ValidationUtil --- .../component_declaration/component_base.dart | 18 +++++++++--------- .../component_base_test.dart | 18 ++++++++++-------- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/lib/src/component_declaration/component_base.dart b/lib/src/component_declaration/component_base.dart index 96b71956c..60178e9de 100644 --- a/lib/src/component_declaration/component_base.dart +++ b/lib/src/component_declaration/component_base.dart @@ -416,7 +416,7 @@ abstract class UiStatefulComponent extends MapView { //Used to customize warning based on whether the data is props or state bool isProps; + String message; + _WarnOnModify(Map componentData, bool isProps): super(componentData){ this.isProps = isProps; } @@ -457,24 +459,22 @@ class _WarnOnModify extends MapView { @override operator []=(K key, V value) { if (isProps) { - window.console.warn(unindent( + message = ''' props["$key"] was updated incorrectly. Never mutate this.props directly, as it can cause unexpected behavior; props must be updated only by passing in new values when rerendering this component. This will throw in UiComponentV2 (to be released as part of the React 16 upgrade). - ''' - )); + '''; } else { - window.console.warn(unindent( + message = ''' - state["$key"] was updated incorrectly. Never this.state directly, as it can cause unexpected behavior; state must be updated only via setState. + state["$key"] was updated incorrectly. Never mutate this.state directly, as it can cause unexpected behavior; state must be updated only via setState. This will throw in UiComponentV2 (to be released as part of the React 16 upgrade). - ''' - )); + '''; } - super[key] = value; + ValidationUtil.warn(message); } } diff --git a/test/over_react/component_declaration/component_base_test.dart b/test/over_react/component_declaration/component_base_test.dart index fff941733..2f1c798d4 100644 --- a/test/over_react/component_declaration/component_base_test.dart +++ b/test/over_react/component_declaration/component_base_test.dart @@ -125,13 +125,14 @@ main() { }, testOn: '!js'); test('warns against setting props directly', () { + startRecordingValidationWarnings(); var instance = render(TestComponent()()); var component = getDartComponent(instance); + var test = () => component.props['id'] = 'test'; - expect(() { - component.props['id'] = 'test'; - }, throwsA(const TypeMatcher())); - }, testOn: '!js'); + test(); + verifyValidationWarning(contains('Never mutate this.props directly')); + }); group('renders a DOM component with the correct children when', () { _commonVariadicChildrenTests(Dom.div()); @@ -838,10 +839,11 @@ main() { }); test('warns against setting state directly', () { - expect(() { - statefulComponent.state['test'] = true; - }, throwsA(const TypeMatcher())); - }, testOn: '!js'); + var changeState = () => statefulComponent.state['test'] = true; + + changeState(); + verifyValidationWarning(contains('Never mutate this.state directly')); + }); }); group('setter:', () { From 841d442f61a149ad2acc30d673169dc954a68c75 Mon Sep 17 00:00:00 2001 From: Joe Bingham Date: Wed, 20 Feb 2019 09:29:52 -0700 Subject: [PATCH 11/15] Stop the recording of validation warnings --- .../component_declaration/component_base_test.dart | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/over_react/component_declaration/component_base_test.dart b/test/over_react/component_declaration/component_base_test.dart index 2f1c798d4..443ca156d 100644 --- a/test/over_react/component_declaration/component_base_test.dart +++ b/test/over_react/component_declaration/component_base_test.dart @@ -128,10 +128,11 @@ main() { startRecordingValidationWarnings(); var instance = render(TestComponent()()); var component = getDartComponent(instance); - var test = () => component.props['id'] = 'test'; + var changeProps = () => component.props['id'] = 'test'; - test(); + changeProps(); verifyValidationWarning(contains('Never mutate this.props directly')); + stopRecordingValidationWarnings(); }); group('renders a DOM component with the correct children when', () { From 4e74454f40adba6dcdf16869e96906c9b7c6d60a Mon Sep 17 00:00:00 2001 From: Joe Bingham Date: Wed, 20 Feb 2019 09:33:34 -0700 Subject: [PATCH 12/15] Start and stop validation utils for state change --- test/over_react/component_declaration/component_base_test.dart | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/over_react/component_declaration/component_base_test.dart b/test/over_react/component_declaration/component_base_test.dart index 443ca156d..ba0fb1301 100644 --- a/test/over_react/component_declaration/component_base_test.dart +++ b/test/over_react/component_declaration/component_base_test.dart @@ -840,10 +840,12 @@ main() { }); test('warns against setting state directly', () { + startRecordingValidationWarnings(); var changeState = () => statefulComponent.state['test'] = true; changeState(); verifyValidationWarning(contains('Never mutate this.state directly')); + stopRecordingValidationWarnings(); }); }); From 92f772277c34fae2350ce162023f53cbccd54ac8 Mon Sep 17 00:00:00 2001 From: Keal Jones <41018730+kealjones-wk@users.noreply.github.com> Date: Wed, 20 Feb 2019 13:25:59 -0700 Subject: [PATCH 13/15] Limit line length Co-Authored-By: joebingham-wk <46691367+joebingham-wk@users.noreply.github.com> --- lib/src/component_declaration/component_base.dart | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/src/component_declaration/component_base.dart b/lib/src/component_declaration/component_base.dart index 60178e9de..4b61625a2 100644 --- a/lib/src/component_declaration/component_base.dart +++ b/lib/src/component_declaration/component_base.dart @@ -461,14 +461,16 @@ class _WarnOnModify extends MapView { if (isProps) { message = ''' - props["$key"] was updated incorrectly. Never mutate this.props directly, as it can cause unexpected behavior; props must be updated only by passing in new values when rerendering this component. + props["$key"] was updated incorrectly. Never mutate this.props directly, as it can cause unexpected behavior; + props must be updated only by passing in new values when re-rendering this component. This will throw in UiComponentV2 (to be released as part of the React 16 upgrade). '''; } else { message = ''' - state["$key"] was updated incorrectly. Never mutate this.state directly, as it can cause unexpected behavior; state must be updated only via setState. + state["$key"] was updated incorrectly. Never mutate this.state directly, as it can cause unexpected behavior; + state must be updated only via setState. This will throw in UiComponentV2 (to be released as part of the React 16 upgrade). '''; From 0be47eb76472ebce1a64b8c1cd095b99f4f76e87 Mon Sep 17 00:00:00 2001 From: Joe Bingham Date: Wed, 20 Feb 2019 13:30:16 -0700 Subject: [PATCH 14/15] Incorporate review feedback --- lib/src/component_declaration/component_base.dart | 5 +---- .../component_declaration/component_base_test.dart | 2 -- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/src/component_declaration/component_base.dart b/lib/src/component_declaration/component_base.dart index 4b61625a2..9dde5c179 100644 --- a/lib/src/component_declaration/component_base.dart +++ b/lib/src/component_declaration/component_base.dart @@ -446,15 +446,12 @@ abstract class UiStatefulComponent extends MapView { - //Used to customize warning based on whether the data is props or state bool isProps; String message; - _WarnOnModify(Map componentData, bool isProps): super(componentData){ - this.isProps = isProps; - } + _WarnOnModify(Map componentData, this.isProps): super(componentData); @override operator []=(K key, V value) { diff --git a/test/over_react/component_declaration/component_base_test.dart b/test/over_react/component_declaration/component_base_test.dart index ba0fb1301..028a61c7a 100644 --- a/test/over_react/component_declaration/component_base_test.dart +++ b/test/over_react/component_declaration/component_base_test.dart @@ -129,7 +129,6 @@ main() { var instance = render(TestComponent()()); var component = getDartComponent(instance); var changeProps = () => component.props['id'] = 'test'; - changeProps(); verifyValidationWarning(contains('Never mutate this.props directly')); stopRecordingValidationWarnings(); @@ -842,7 +841,6 @@ main() { test('warns against setting state directly', () { startRecordingValidationWarnings(); var changeState = () => statefulComponent.state['test'] = true; - changeState(); verifyValidationWarning(contains('Never mutate this.state directly')); stopRecordingValidationWarnings(); From 80ac6caf35f73fd9fc337f292fbbda6745c66c2a Mon Sep 17 00:00:00 2001 From: Joe Bingham Date: Thu, 21 Feb 2019 14:14:59 -0700 Subject: [PATCH 15/15] Wrap message in unindent --- lib/src/component_declaration/component_base.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/src/component_declaration/component_base.dart b/lib/src/component_declaration/component_base.dart index 9dde5c179..ef04069ac 100644 --- a/lib/src/component_declaration/component_base.dart +++ b/lib/src/component_declaration/component_base.dart @@ -473,7 +473,7 @@ class _WarnOnModify extends MapView { '''; } super[key] = value; - ValidationUtil.warn(message); + ValidationUtil.warn(unindent(message)); } }