-
Notifications
You must be signed in to change notification settings - Fork 14
Add context support #134
Comments
From @catmando on March 28, 2016 15:14 I would request that we keep the syntax consistent with params and state: params.foo Also I am not clear on what the type is for, does it have the same purpose as the optional typing of params? If so again I would want to see consistent syntax. |
From @wied03 on March 28, 2016 15:16 You are required to give React a proptype for context. My code converts the Ruby type into a proptype. You could just give it an |
From @wied03 on March 28, 2016 15:20 It looks like |
From @catmando on March 28, 2016 15:33 context.foo vs. context[:foo] is just syntax sugar right? it can do the same thing under the hood. Interesting that React requires a proptype. So do we want to be consistent and make it optional (and if not provided we will just use Object as you suggest.) So syntax would be consistent with params
|
From @catmando on March 28, 2016 15:35 I realized I don't have the initializer in the above, but again would like to make it all consistent with param for ease of learning. |
From @wied03 on March 28, 2016 15:37 What about the caching question? |
From @catmando on March 28, 2016 15:55 right... so we want to redo the PropsWrapper (and validation too) so that it works for props, and context, plus it work for class level props, and special param types (like observables) without adding burden to the base function if you don't use those features... |
From @wied03 on March 28, 2016 15:56 And I'm cool with doing a PR for this since I already wrote it in react-opal. I'm mainly curious about the cache question now. |
From @ajjahn on March 29, 2016 2:56 @wied03 That cache was added to address #124 and #125. @catmando You implemented a fix for the 0.7.x branch (in a different way). When I pulled it into master things had changed, so I brought the spec you wrote along and made it pass. Those issues are only issues because of the @wied03 We've had discussions on the state and props wrappers. I believe they both could use some TLC. Especially the implementation of state and observables. Too many objects know too much about each other. |
From @ajjahn on March 29, 2016 15:50 @wied03 The cache for a prop busts if the underlying component's prop has changed. There isn't a need to mark the entire cache as dirty. When the prop is accessed (regardless of renders), the current value of the prop is examined. Really, the argument could be made that this caching is a concern of the object that responds to |
From @wied03 on March 29, 2016 15:55 I'm guessing that the complexity of the cache is worth it? Was it really that expensive to do the camel case/native conversions on demand? |
From @ajjahn on March 29, 2016 16:28 @wied03 I don't believe camel case/native conversion was the primary reason. The problem here is react.rb has some reactive-record feature envy. Reactive record is counting on react components to deserialize it's models that is is passing as props. I'd like to see serialization/deserialization removed. It belongs elsewhere. So as @catmando suggested, it would be helpful to converge on a consensues in #96. |
on hold (I think) until we get #96 figured out. |
From @wied03 on March 25, 2016 21:15
In react-opal, you can use the React context feature, like this. I can probably do a PR for this.
Copied from original issue: zetachang/react.rb#134
The text was updated successfully, but these errors were encountered: