Repository navigation
Conversation
|
@ocrickard updated the pull request - view changes |
| view:(const CKComponentViewConfiguration &)view | ||
| size:(const CKComponentSize &)size; | ||
|
|
||
| @end |
There was a problem hiding this comment.
This is just here as a demonstration. In order to provide type-safety, you have to provide your own +new that does a pass-through to the parent's +new method.
| } | ||
| private: | ||
| T _value; | ||
| }; |
There was a problem hiding this comment.
I'm liking this pattern. I think we should formalize it as the recommended way to provide required props. I dislike the mixed named Obj-C param/options structs as our components have grown more complex.
|
|
||
| private: | ||
| std::shared_ptr<placeholder> _content; | ||
| }; |
There was a problem hiding this comment.
This is basically boost::any. It copies any struct to the heap, and allows implicit conversion to the underlying type. Conversion to any type will successfully compile, but when done at runtime, may fail with a failed exception.
| return [self newWithPropsStruct:props | ||
| view:view | ||
| size:size]; | ||
| } |
There was a problem hiding this comment.
Sucks we have to do this passthrough. Anyone have ideas on how to remove it while maintaining type-safety?
| size:size]; | ||
| } | ||
|
|
||
| + (CKComponent *)renderWithProps:(const CKTypedComponentStruct &)p |
There was a problem hiding this comment.
The subclass should implement this method, but is forced to unwrap the props. I don't think that's too big of a deal since it's internal to the component, and we currently do this for id type to state already, so it's not a dealbreaker for me.
| view:(const CKComponentViewConfiguration &)view | ||
| size:(const CKComponentSize &)size; | ||
|
|
||
| @property (nonatomic, assign, readonly) CKTypedComponentStruct props; |
There was a problem hiding this comment.
@ocrickard, how about making this property an std::shared_ptr< CKTypedComponentStruct> where CKTypedComponentStruct<T> is a very simple templated structure that just stores a member of type T (no handling optionals).
The shared_ptr itself should be able to represent null as a default value, *props == nullptr.
There was a problem hiding this comment.
Well, Obj-C code doesn't have support for templates. We have to only interact with specializations of templates within Obj-C interfaces. This is why all this complicated logic exists.
Is there a way to support CKTypedComponentStruct?
There was a problem hiding this comment.
Ah, right. We could just make CKTypedComponentStruct an empty c++ struct and all other concrete propTypes inherit that.
There was a problem hiding this comment.
Yep, that's almost exactly what we're doing here. Sadly, that allows you to pass any subclass of the component struct to any component...
There was a problem hiding this comment.
:/ I don't see a good way to get the type-safety this without making the entire component a C++ object inheriting from say CK::Component.
There was a problem hiding this comment.
+1, I can't think of any way of making this type-safe as an objc class.
We could make them actual c++ classes, then have objc wrappers around them, but that seems like it would affect binary size. 😢
There was a problem hiding this comment.
@ryanrhee It would really increase the binary size right? I'm thinking we'd have exactly one ObjC wrapper class that wraps any subclass of CK::Component.
|
How come you removed the macros? |
|
@ocrickard I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project. |
|
Alright, I'm closing this for now. Let's revive it down the road if we want to take on the project. |
This is an RFC that allows a formalized C++ prop component. This lets us move to an implementation of composite components to be implemented as +render instead of +new, which will force people to avoid storing into ivars, and removes the necessity to use component scope in client code to use state.
This brings us a little closer to React, and provides an opportunity for automatic memoization.