-
Notifications
You must be signed in to change notification settings - Fork 67
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
JS-object-based props/state, Component2 initial impl, JsMap #161
JS-object-based props/state, Component2 initial impl, JsMap #161
Conversation
# Conflicts: # example/test/react_test_components.dart # lib/react.dart # lib/react_client.dart # lib/react_client/react_interop.dart # test/factory/common_factory_tests.dart # test/lifecycle_test.dart
…rt_1 Also add polyfills to build # Conflicts: # example/test/react_test_components.dart # js_src/dart_helpers.js # lib/react.js # lib/react_client/react_interop.dart # lib/react_prod.js # lib/react_with_addons.js # lib/react_with_react_dom_prod.js # test/ReactSetStateTestComponent.js # test/lifecycle_test.dart
# Conflicts: # package.json
1351650
to
ee377ec
Compare
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.
Additional feedback here: greglittlefield-wf#2
@@ -7,7 +7,8 @@ library react; | |||
|
|||
import 'package:react/src/typedefs.dart'; | |||
|
|||
typedef Component ComponentFactory(); | |||
// todo revert this? | |||
typedef T ComponentFactory<T extends Component>(); |
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 don't see any reason to revert it... its not a breaking API change as far as I can tell....
…view CR feedback for JS map stuff
fa614be
to
ee873c1
Compare
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.
Looks super good! so excited, just some nits and questions.
@override | ||
bool containsKey(Object key) { | ||
// if (!_isValidKey(key)) return false; | ||
// assert(_isValidKey(key)); |
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.
and again, more guff
|
||
// overridden for to avoid cast in case the implementation of keys changes | ||
// @override | ||
// int get length => _Object.keys(jsObject).length; |
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.
from line 99 to here all can probably go, unless the worry of implementation change is still relevant then id probably un-comment it out.
/// | ||
/// Will be removed when [Component] is removed in the `6.0.0` release. | ||
@Deprecated('6.0.0') | ||
void replaceState(Map newState, [SetStateCallback callback]) => |
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.
Why is this deprecated in Component2
and not in Component
? why do we even need this in Component2
?
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.
Good point, it should probably be deprecated in Component, along with any other non-lifecycle members that will be unsupported in Component2.
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 will add "non lifecycle members of Component
" to our deprecation ticket.
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 say we do that in the PR where we add all the other deprecations
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.
+10
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.
+1
Changes
Initial implementation of JS-object-based props/state
Component2
base classprops
object, as opposed to within a Dart object stored as aninternal
propstate
object, as opposed to completely within the Dart layerAdvantages:
JsBackedMap
A Dart class that adapts JS objects to the Dart
Map
interface, allowing properties to be easily read/written directly.Component2
's fields and lifecycle methodsReactDomComponentFactory
that served similar functions, removing an extra copy in DOMReactElement
creationEtc.
Component2
Testing