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.

Sign up for GitHub

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 15 commits into from
Feb 21, 2019
Merged

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f41c9f3
Add a class watching for direct props/state mutation
joebingham-wk Feb 15, 2019
3caa5dd
Change formatting
joebingham-wk Feb 15, 2019
b01e4c8
Change the Assert condition
joebingham-wk Feb 18, 2019
95e6b72
Add tests to check if the assert is thrown
joebingham-wk Feb 18, 2019
f459e90
Fix formatting
joebingham-wk Feb 18, 2019
c1237c6
Update testing to pass CI tests
joebingham-wk Feb 18, 2019
84c1e40
Exclude .js from dev only tests
joebingham-wk Feb 18, 2019
c15e0b7
Remove extra line
joebingham-wk Feb 18, 2019
c0dc513
Update message and remove assert
joebingham-wk Feb 19, 2019
006340b
Update tests to use ValidationUtil
joebingham-wk Feb 20, 2019
841d442
Stop the recording of validation warnings
joebingham-wk Feb 20, 2019
4e74454
Start and stop validation utils for state change
joebingham-wk Feb 20, 2019
92f7722
Limit line length
kealjones-wk Feb 20, 2019
0be47eb
Incorporate review feedback
joebingham-wk Feb 20, 2019
80ac6ca
Wrap message in unindent
joebingham-wk Feb 21, 2019
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
16 changes: 14 additions & 2 deletions lib/src/component_declaration/component_base.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,7 +245,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(WarnOnModify(unwrappedProps));
aaronlademann-wf marked this conversation as resolved.
Show resolved Hide resolved
_typedPropsCache[unwrappedProps] = typedProps;
}
return typedProps;
Expand Down Expand Up @@ -415,7 +415,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(WarnOnModify(unwrappedState));
_typedStateCache[unwrappedState] = typedState;
}
return typedState;
Expand Down Expand Up @@ -444,6 +444,18 @@ abstract class UiStatefulComponent<TProps extends UiProps, TState extends UiStat
// ----------------------------------------------------------------------
}

class WarnOnModify<K, V> extends MapView<K, V> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be private (add an underscore to the beginning of the name).

WarnOnModify(Map componentData): super(componentData);

@override
operator []=(K key, V value) {
assert(
aaronlademann-wf marked this conversation as resolved.
Show resolved Hide resolved
false,
'Mutating the state or props directly will soon be impossible without causing breakages.'
aaronlademann-wf marked this conversation as resolved.
Show resolved Hide resolved
);
super[key] = value;
}
}

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

test('warns against setting props directly', () {
var instance = render(TestComponent()());
var component = getDartComponent(instance);

expect(() {
component.props['id'] = 'test';
}, throwsA(const TypeMatcher<AssertionError>()));
}, testOn: '!js');

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

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

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

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

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

test('warns against setting state directly', () {
expect(() {
statefulComponent.state['test'] = true;
}, throwsA(const TypeMatcher<AssertionError>()));
}, testOn: '!js');
});

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