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.

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

Add context support #134

Open
wied03 opened this issue Mar 25, 2016 · 19 comments
Open

Add context support #134

wied03 opened this issue Mar 25, 2016 · 19 comments

Comments

@wied03
Copy link
Contributor

wied03 commented Mar 25, 2016

In react-opal, you can use the React context feature, like this. I can probably do a PR for this.

class ParentComponent
  include React::Component

  # Simply supply the Ruby/Opal type that :foo will be and react-opal will map that to a React PropType automatically
  provide_context(:foo, Fixnum) { params[:foo_value] }

  def render
    present ChildComponent
  end
end

class ChildComponent
  include React::Component

  consume_context(:foo, Fixnum)

  def render
    # will render the :foo_value prop passed into ParentComponent
    div { "foo is #{self.context[:foo]}" } 
  end
end
@ajjahn
Copy link
Collaborator

ajjahn commented Mar 28, 2016

@wied03 Please do!

@catmando
Copy link
Collaborator

I would request that we keep the syntax consistent with params and state:

params.foo
state.foo
context.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.

@wied03
Copy link
Contributor Author

wied03 commented Mar 28, 2016

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 Object type for everything but I figured I would try and play nice.

@wied03
Copy link
Contributor Author

wied03 commented Mar 28, 2016

@catmando - As to context.foo vs. context[:foo], I guess you guys changed react.rb after my fork to return a method missing type class instead of a Hash for params to allow params.foo ?

@wied03
Copy link
Contributor Author

wied03 commented Mar 28, 2016

It looks like PropsWrapper is being used for that. Why do the cache? To avoid camel_casing, etc. again? How are you marking the cache dirty when a re-render happens?

@catmando
Copy link
Collaborator

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

provide_context :foo, type: Bar or provide_context :foo # equivilent to adding, type: Object

@catmando
Copy link
Collaborator

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.

@wied03
Copy link
Contributor Author

wied03 commented Mar 28, 2016

What about the caching question?

@catmando
Copy link
Collaborator

I'm leaving that to @ajjahn. I'm just going from the standpoint that there is no reason why we can't make context.foo work the same as context[:foo]. In fact it should be easier if we wanted to work on it to make context.foo slightly faster than context[:foo].

@wied03
Copy link
Contributor Author

wied03 commented Mar 28, 2016

@catmando - I agree there, my mind moved on to the internals of PropsWrapper

@catmando
Copy link
Collaborator

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...

@wied03
Copy link
Contributor Author

wied03 commented Mar 28, 2016

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.

@ajjahn
Copy link
Collaborator

ajjahn commented Mar 29, 2016

@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 _react_params_conversion, which I feel is a code smell anyway, and not a responsibility of react.

@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.

@wied03
Copy link
Contributor Author

wied03 commented Mar 29, 2016

@ajjahn - I'm not really as curious about why the cache is there as much as how you know when to clean it out. If you have a re-render of a component, doesn't the cache as is stick around?

@catmando
Copy link
Collaborator

okay so if part of the confusion is being caused by _react_params_conversion, then lets wrap up that discussion. ( #96 )

@ajjahn
Copy link
Collaborator

ajjahn commented Mar 29, 2016

@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 _react_params_conversion. It should memoize it's converted value, and return it on subsequent calls. If a new prop arrives that responds to _react_params_conversion, it's value will not yet have been memoized, and the conversion will take place.

@wied03
Copy link
Contributor Author

wied03 commented Mar 29, 2016

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?

@ajjahn
Copy link
Collaborator

ajjahn commented Mar 29, 2016

@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.

@sollycatprint
Copy link

This issue was moved to ruby-hyperloop/hyper-react#134

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants