-
Notifications
You must be signed in to change notification settings - Fork 58
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-16997 Fix selector hooks redrawing when values haven't changed #730
Merged
Merged
Changes from 19 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
64d087e
Fix selectors always returning new objects by conditionally wrapping …
greglittlefield-wf c01d310
Reuse wrapped functions
greglittlefield-wf 2c68978
Add regression tests
greglittlefield-wf 5e12f20
Fix missing arg causing swallowed RTEs
greglittlefield-wf 4dfb35d
Fix missing arg causing swallowed RTEs for createSelectorHook
greglittlefield-wf 656cfca
Add some shared tests for different selected value types
greglittlefield-wf df95525
Add test coverage for createSelector, add null case and additional se…
greglittlefield-wf d472036
Use typed props
greglittlefield-wf 498c594
Fix typo
greglittlefield-wf 2e74a56
Update typing from dynamic to Object to prevent unintentional implici…
greglittlefield-wf 4202bc5
Revert changes in old tests
greglittlefield-wf f4f3ae4
Remove unused dependency
greglittlefield-wf 5973053
Add more test coverage
greglittlefield-wf 2880847
Cleanup
greglittlefield-wf 7cee6c0
Add test coverage for value identity
greglittlefield-wf abe3425
Add additional test coverage for connect
greglittlefield-wf e7cabbd
Add comment
greglittlefield-wf b199497
Add useSelector example
greglittlefield-wf b55e993
Update doc comment to not get flagged by dependency_validator
greglittlefield-wf cb65a0e
Fix copyright headers
greglittlefield-wf File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,13 +20,13 @@ import 'dart:html'; | |
import 'dart:js_util' as js_util; | ||
|
||
import 'package:js/js.dart'; | ||
import 'package:memoize/memoize.dart'; | ||
import 'package:meta/meta.dart'; | ||
import 'package:over_react/component_base.dart'; | ||
import 'package:over_react/src/component_declaration/annotations.dart'; | ||
import 'package:over_react/src/component_declaration/builder_helpers.dart' as builder_helpers; | ||
import 'package:over_react/src/component_declaration/component_type_checking.dart'; | ||
import 'package:over_react/src/util/context.dart'; | ||
import 'package:over_react/src/util/dart_value_wrapper.dart'; | ||
import 'package:over_react/src/util/equality.dart'; | ||
import 'package:react/react_client.dart'; | ||
import 'package:react/react_client/js_backed_map.dart'; | ||
|
@@ -35,6 +35,10 @@ import 'package:redux/redux.dart'; | |
|
||
part 'over_react_redux.over_react.g.dart'; | ||
|
||
// Notes: | ||
// | ||
// [1] This value could be either a raw value or a value wrapped in DartValueWrapper. | ||
|
||
/// This class is present: | ||
/// | ||
/// 1. to allow for consumers which have used the --backwards-compat flag with over_react_codemod to statically analyze: | ||
|
@@ -200,47 +204,47 @@ UiFactory<TProps> Function(UiFactory<TProps>) connect<TReduxState, TProps extend | |
return interopFunction; | ||
} | ||
|
||
JsMap handleMapStateToProps(ReactInteropValue jsState) { | ||
JsMap handleMapStateToProps(Object /*[1]*/ jsState) { | ||
return jsMapFromProps( | ||
mapStateToProps( | ||
unwrapInteropValue(jsState), | ||
DartValueWrapper.unwrapIfNeeded(jsState), | ||
), | ||
); | ||
} | ||
|
||
JsMap handleMapStateToPropsWithOwnProps(ReactInteropValue jsState, JsMap jsOwnProps) { | ||
JsMap handleMapStateToPropsWithOwnProps(Object /*[1]*/ jsState, JsMap jsOwnProps) { | ||
return jsMapFromProps( | ||
mapStateToPropsWithOwnProps( | ||
unwrapInteropValue(jsState), | ||
DartValueWrapper.unwrapIfNeeded(jsState), | ||
jsPropsToTProps(jsOwnProps), | ||
), | ||
); | ||
} | ||
|
||
JsMap Function(ReactInteropValue jsState) handleMakeMapStateToProps(ReactInteropValue initialJsState, JsMap initialJsOwnProps) { | ||
JsMap Function(Object /*[1]*/ jsState) handleMakeMapStateToProps(Object /*[1]*/ initialJsState, JsMap initialJsOwnProps) { | ||
var mapToFactory = makeMapStateToProps( | ||
unwrapInteropValue(initialJsState), | ||
DartValueWrapper.unwrapIfNeeded(initialJsState), | ||
jsPropsToTProps(initialJsOwnProps) | ||
); | ||
JsMap handleMakeMapStateToPropsFactory(ReactInteropValue jsState) { | ||
JsMap handleMakeMapStateToPropsFactory(Object /*[1]*/ jsState) { | ||
return jsMapFromProps( | ||
mapToFactory( | ||
unwrapInteropValue(jsState), | ||
DartValueWrapper.unwrapIfNeeded(jsState), | ||
), | ||
); | ||
} | ||
return allowInteropWithArgCount(handleMakeMapStateToPropsFactory, 1); | ||
} | ||
|
||
JsMap Function(ReactInteropValue jsState, JsMap jsOwnProps) handleMakeMapStateToPropsWithOwnProps(ReactInteropValue initialJsState, JsMap initialJsOwnProps) { | ||
JsMap Function(Object /*[1]*/ jsState, JsMap jsOwnProps) handleMakeMapStateToPropsWithOwnProps(Object /*[1]*/ initialJsState, JsMap initialJsOwnProps) { | ||
var mapToFactory = makeMapStateToPropsWithOwnProps( | ||
unwrapInteropValue(initialJsState), | ||
DartValueWrapper.unwrapIfNeeded(initialJsState), | ||
jsPropsToTProps(initialJsOwnProps) | ||
); | ||
JsMap handleMakeMapStateToPropsWithOwnPropsFactory(ReactInteropValue jsState, JsMap jsOwnProps) { | ||
JsMap handleMakeMapStateToPropsWithOwnPropsFactory(Object /*[1]*/ jsState, JsMap jsOwnProps) { | ||
return jsMapFromProps( | ||
mapToFactory( | ||
unwrapInteropValue(jsState), | ||
DartValueWrapper.unwrapIfNeeded(jsState), | ||
jsPropsToTProps(jsOwnProps), | ||
), | ||
); | ||
|
@@ -304,8 +308,8 @@ UiFactory<TProps> Function(UiFactory<TProps>) connect<TReduxState, TProps extend | |
); | ||
} | ||
|
||
bool handleAreStatesEqual(ReactInteropValue jsNext, ReactInteropValue jsPrev) => | ||
areStatesEqual(unwrapInteropValue(jsNext), unwrapInteropValue(jsPrev)); | ||
bool handleAreStatesEqual(Object /*[1]*/ jsNext, Object /*[1]*/ jsPrev) => | ||
areStatesEqual(DartValueWrapper.unwrapIfNeeded(jsNext), DartValueWrapper.unwrapIfNeeded(jsPrev)); | ||
|
||
bool handleAreOwnPropsEqual(JsMap jsNext, JsMap jsPrev) => | ||
areOwnPropsEqual(jsPropsToTProps(jsNext), jsPropsToTProps(jsPrev)); | ||
|
@@ -497,14 +501,9 @@ class ReactJsReactReduxComponentFactoryProxy extends ReactJsContextComponentFact | |
|
||
/// Converts a Redux.dart [Store] into a Javascript object formatted for consumption by react-redux. | ||
JsReactReduxStore _reduxifyStore(Store store) { | ||
// Memoize this so that the same ReactInteropValue instances will be used | ||
// for a given state, allowing JS `===` checks to not fail when the same | ||
// state object is passed. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we pass through the value directly (and memoize it in the cases we wrap it in |
||
final memoizedWrapInteropValue = imemo1(wrapInteropValue); | ||
|
||
return JsReactReduxStore( | ||
getState: allowInterop(() { | ||
return memoizedWrapInteropValue(store.state); | ||
return DartValueWrapper.wrapIfNeeded(store.state); | ||
}), | ||
subscribe: allowInterop((cb) { | ||
return allowInterop(store.onChange.listen((_){cb();}).cancel); | ||
|
@@ -527,7 +526,7 @@ class JsReactReduxStore { | |
external Store get dartStore; | ||
|
||
external factory JsReactReduxStore({ | ||
ReactInteropValue Function() getState, | ||
Object /*[1]*/ Function() getState, | ||
Dispatcher dispatch, | ||
Function Function(Function) subscribe, | ||
Store dartStore, | ||
|
@@ -538,22 +537,22 @@ class JsReactReduxStore { | |
@JS() | ||
@anonymous | ||
class JsConnectOptions { | ||
external set areStatesEqual(bool Function(ReactInteropValue, ReactInteropValue) value); | ||
external set areStatesEqual(bool Function(Object /*[1]*/, Object /*[1]*/) value); | ||
external set areOwnPropsEqual(bool Function(JsMap, JsMap) value); | ||
external set areStatePropsEqual(bool Function(JsMap, JsMap) value); | ||
external set areMergedPropsEqual(bool Function(JsMap, JsMap) value); | ||
external set forwardRef(bool value); | ||
external set pure(bool value); | ||
external set context(ReactContext value); | ||
external bool Function(ReactInteropValue, ReactInteropValue) get areStatesEqual; | ||
external bool Function(Object /*[1]*/, Object /*[1]*/) get areStatesEqual; | ||
external bool Function(JsMap, JsMap) get areOwnPropsEqual; | ||
external bool Function(JsMap, JsMap) get areStatePropsEqual; | ||
external bool Function(JsMap, JsMap) get areMergedPropsEqual; | ||
external bool get forwardRef; | ||
external bool get pure; | ||
external ReactContext get context; | ||
external factory JsConnectOptions({ | ||
bool Function(ReactInteropValue, ReactInteropValue) areStatesEqual, | ||
bool Function(Object /*[1]*/, Object /*[1]*/) areStatesEqual, | ||
bool Function(JsMap, JsMap) areOwnPropsEqual, | ||
bool Function(JsMap, JsMap) areStatePropsEqual, | ||
bool Function(JsMap, JsMap) areMergedPropsEqual, | ||
|
@@ -563,17 +562,3 @@ class JsConnectOptions { | |
}); | ||
} | ||
|
||
/// A wrapper class that prevents dart2js from jsifying [value]. | ||
class ReactInteropValue { | ||
dynamic value; | ||
} | ||
|
||
/// A helper function that retrieves the [value] from a [ReactInteropValue]. | ||
T unwrapInteropValue<T>(ReactInteropValue value) { | ||
return value.value as T; | ||
} | ||
|
||
/// A helper function that wraps a [value] in a [ReactInteropValue]. | ||
ReactInteropValue wrapInteropValue(dynamic value) { | ||
return ReactInteropValue()..value = value; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import 'dart:js'; | ||
|
||
// Taken from react-dart | ||
/// A wrapper around a value that can't be stored in its raw form | ||
/// within a JS object (e.g., a Dart function). | ||
class DartValueWrapper { | ||
final Object value; | ||
|
||
const DartValueWrapper(this.value); | ||
|
||
static final _functionWrapperCache = Expando<DartValueWrapper>('_functionWrapperCache'); | ||
|
||
static Object wrapIfNeeded(Object value) { | ||
// This case should be fairly uncommon, since functions usually aren't used as | ||
// a Redux store's state or the result of a connect or selector hook selector. | ||
if (value is Function && !identical(allowInterop(value), value)) { | ||
// Reuse wrappers for the same function so that they're identical to the JS. | ||
return _functionWrapperCache[value] ??= DartValueWrapper(value); | ||
} | ||
return value; | ||
} | ||
|
||
static T unwrapIfNeeded<T>(Object value) { | ||
if (value is DartValueWrapper) { | ||
return value.value as T; | ||
} | ||
return value as T; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this to work around dependency_validator flagging imports in comments