-
Notifications
You must be signed in to change notification settings - Fork 68
CPLAT-8036 Add useContext Hook #237
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
Conversation
b7cab0d
to
d65d845
Compare
react.registerFunctionComponent(UseContextTestComponent, displayName: 'useContextTest'); | ||
|
||
UseContextTestComponent(Map props) { | ||
final context = useContext(TestNewContext); |
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.
It bothers me a bit that context
will be dynamic unless you provide a type on the left side, and even then - there'd be no way to ensure that the type doesn't mismatch with the underlying type that was returned by createContext
. Makes me wonder what the point of the generic parameter is on createContext
.
By making these changes locally... all the typing seems to line up and makes context
correctly inferred as a Map
in this situation as a result of createContext<Map>
being called on line 63.
// context.dart
- class Context {
+ class Context<T> {
// ...
}
- Context createContext<TValue>([
+ Context<TValue> createContext<TValue>([
// ...
// hooks.dart
- dynamic useContext(Context context) => // ...
+ T useContext<T>(Context<T> context) => // ...
@greglittlefield-wf @kealjones-wk @joebingham-wk is there some reason that I forgot about that led us to omit generic typing from the Context
class?
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.
Honestly I don't recall, I feel like there would have been a reason because the Context
object in OverReact is typed exactly like that and i don't know why i would have omitted it in React-Dart unless it was just a minor oversight (very likely). @joebingham-wk if you can add this typing id say do it, but test the local context examples in browser and test it with over_react (tests & real examples if already in place) to make sure it doesn't break anything (i don't think it should).
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
Co-Authored-By: Greg Littlefield <[email protected]>
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
Motivation
Expose the
useContext
hook so that functional components can tap into it.Changes
useContext
functionTesting
Please Review @greglittlefield-wf @kealjones-wk @aaronlademann-wf @sydneyjodon-wk