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

CPLAT-3616 Warn consumers about props / state mutation #249

Merged
Merged
36 changes: 34 additions & 2 deletions lib/src/component_declaration/component_base.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -245,7 +246,7 @@ abstract class UiComponent<TProps extends UiProps> extends react.Component imple
var unwrappedProps = this.unwrappedProps;
var typedProps = _typedPropsCache[unwrappedProps];
if (typedProps == null) {
typedProps = typedPropsFactory(unwrappedProps);
typedProps = typedPropsFactory(inReactDevMode ? _WarnOnModify(unwrappedProps, true) : unwrappedProps);
_typedPropsCache[unwrappedProps] = typedProps;
}
return typedProps;
Expand Down Expand Up @@ -415,7 +416,7 @@ abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiStat
var unwrappedState = this.unwrappedState;
var typedState = _typedStateCache[unwrappedState];
if (typedState == null) {
typedState = typedStateFactory(unwrappedState);
typedState = typedStateFactory(inReactDevMode ? _WarnOnModify(unwrappedState, false) : unwrappedState);
_typedStateCache[unwrappedState] = typedState;
}
return typedState;
Expand Down Expand Up @@ -444,6 +445,37 @@ abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiStat
// ----------------------------------------------------------------------
}

class _WarnOnModify<K, V> extends MapView<K, V> {
//Used to customize warning based on whether the data is props or state
bool isProps;

String message;

_WarnOnModify(Map componentData, this.isProps): super(componentData);

@override
operator []=(K key, V value) {
if (isProps) {
message =
'''
props["$key"] was updated incorrectly. Never mutate this.props directly, as it can cause unexpected behavior;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this currently print out to the console all indented? If so, we should wrap it in an unindent.

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.

This will throw in UiComponentV2 (to be released as part of the React 16 upgrade).
''';
}
super[key] = value;
ValidationUtil.warn(unindent(message));
}
}

/// A `dart.collection.MapView`-like class with strongly-typed getters/setters for React state.
///
Expand Down
20 changes: 19 additions & 1 deletion test/over_react/component_declaration/component_base_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,16 @@ main() {
_commonNonInvokedBuilderTests(Dom.div());
}, testOn: '!js');

test('warns against setting props directly', () {
startRecordingValidationWarnings();
var instance = render(TestComponent()());
var component = getDartComponent(instance);
var changeProps = () => component.props['id'] = 'test';
changeProps();
verifyValidationWarning(contains('Never mutate this.props directly'));
stopRecordingValidationWarnings();
});

group('renders a DOM component with the correct children when', () {
_commonVariadicChildrenTests(Dom.div());

Expand Down Expand Up @@ -800,7 +810,7 @@ main() {

setUp(() {
statefulComponent = new TestStatefulComponentComponent();
statefulComponent.unwrappedState = {};
statefulComponent.unwrappedState = {'test': true};
});

group('`state`', () {
Expand All @@ -827,6 +837,14 @@ main() {

expect(stateBeforeChange, isNot(same(stateAfterChange)));
});

test('warns against setting state directly', () {
startRecordingValidationWarnings();
var changeState = () => statefulComponent.state['test'] = true;
changeState();
verifyValidationWarning(contains('Never mutate this.state directly'));
stopRecordingValidationWarnings();
});
});

group('setter:', () {
Expand Down