API Improvements

[arunoda]

The API looks but weird compared with usual react APIs.

We normally don't do React.cloneElement. It's not a good API pattern. Instead simply accept the class of the component via a prop.

<ListContainer component={MyComponent} ... />

I also like to have a pure JS API where we can use inside Mantra. May be I can help with that.

[SachaG]

Good points! I initially passed the component as prop like you said, but then I had use cases where I also wanted to pass props to the child component:

<ListContainer component={<MyComponent prop1="foo" prop2="bar"/>} ... />

So React.cloneElement is the best technique I found for that. It also seemed more intuitive to just compose components like this, but if it's not a good pattern then we can change it.

And yes, I also think a pure JS API would be better. I just decided to write this using "simple" JSX first, and then once it works think about refactoring it using React Komposer :)

[arunoda]

@SachaG If you wanna pass custom props, then pass it to the container and then pass all of them to the component as well.

The thing is this API looks weird:

<ListContainer collection={Posts} publication="posts.list">
  <PostList/>
</ListContainer>

Anyway, if you use react-komposer internally, it'll do this for you. (Anyway, creating it your own also not that hard).

[SachaG]

So you mean <ListContainer component={MyComponent} propsToPass={myProps} />? I also tried that but I thought the cloneElement thing would be more "React-y". What's so weird about it?

(but yes, we can use react-komposer anyway :)

[arunoda]

Here, you create an element of . So, when you reading it, we don't know how it's get data.

<ListContainer collection={Posts} publication="posts.list">
  <PostList/>
</ListContainer>

Anyway, that's how I feel it.

[yched]

As I wrote in a comment on https://www.discovermeteor.com/blog/data-loading-react, I developped fairly similar custom wrapper components for the project I'm working on.

The issue I see with the current API :

<DocumentContainer collection={Posts} selector={{_id: myId}}>
  <Post />
</DocumentContainer>

is that this forces you to code the Post component with its 'document' prop marked as non required, since by the time React parses the <Post/> jsx and creates the element, the prop is missing.

That is conceptually incorrect, since typically the Post component does need a post, and can actually rely on it being present, since it will never be called until there is one.

The only workaround I found was to code my (equivalent of) DocumentContainer that way :

<DocumentContainer collection={Posts} selector={{_id: myId}}>
  {post => <Post post={post} {...otherPropsIfNeeded} />}
</DocumentContainer>

That is, my DocumentContainer treats its props.children as a function rather than a React element (react-motion, for instance, does something similar), and calls it with the document fetched from Minimongo.

It's a tiny bit less nice to write, but it lets the Post component name its prop the way it wants (e.g 'post' rather than 'document'), and make it required.

Meaning, Post can be a true "dumb" component, and doesn't have to indulge constraints imposed by the DocumentContainer that might wrap it. You can use DocumentContainer to wrap any element, and the "burden" of using DocumentContainer (i.e writing the closure) is put on the code that choses to use the DocumentContainer wrapper, which is only right.

[SachaG]

@yched you're right, which is why you can now also do <DocumentContainer component={Foo} componentProps={bar}/>

[yched]

@SachaG Oh, OK, didn't notice that.

Foo is still required to name its main prop 'document' though. Unless DocumentContainer accepts another prop for 'documentPropName' - at which point I find my closure syntax easier to read and write ;-)

[edit : arguably the closure syntax also kind of faithfully reflects what the DocumentContainer actually does : "when I get a 'post' document, then you'll get a <Post post={post} /> component"]

[SachaG]

Oh right. That's also a good point, but is it such a pain to have to use document? I guess I'm not sure if being able to use any prop name is worth the added complexity?

[yched]

Well, I usually find it nicer to write my dumb components with prop names that makes sense for what the prop contains (or at least for how the dumb component sees it), and not seeing it as just a "document that comes from Minimongo"

Not that big of a deal, but being able to use DocumentContainer with any child component is kind of handy in the long run.

[SachaG]

Hmm, so how about this?

<DocumentContainer component={Foo} componentProps={bar} documentPropName="post"/>

[yched]

Between :

<DocumentContainer collection={Posts} selector={{_id: myId}} component={Post} componentProps={otherProps} documentPropName="post" />

and :

<DocumentContainer collection={Posts} selector={{_id: postId}}>
  {post => <Post post={post} {...otherProps} />}
</DocumentContainer>

I actually find the latter easier to understand. "When the DocumentContainer gets the post with id postId in the Posts collection, you'll get a <Post> with the post"

This also lets you wrap full element trees rather than just a component :

<DocumentContainer collection={Posts} selector={{_id: postId}}>
  {post => 
    <div>
      <Foo post={post} ... />
      <Bar post={post} ... />
    </div
  }
</DocumentContainer>

which can come handy in some cases

[genyded]

+1 for this approach. Also I haven't had time to dig into it yet, but documentPropName does seem to work. If I use document - it's fine, but if I use 'documentPropName'... it stays at Loading... and no data shows up (even though data is being returned with the pass in documentPropName)

[SachaG]

@genyded that's weird, are you using the latest version? I thought I fixed that bug here:

https://github.com/meteor-utilities/react-list-container/commit/ca6a26e7804d177ddc127ff5de9071cdc20f3193#diff-03efc6d9467ff1fa98eb5f858b1f26bdR63

[genyded]

Thanks - Updated to latest and docProp works. Still would like to see what @yched proposed for the API though.

[SachaG]

I'd like to wait a little until we have more votes one way or another before changing the API. Until then I'd rather err on the site of being easy to understand for now.

[zeroasterisk]

Both seem like they are easy enough, but I vote for the nested closure, for basic readability (IMO)

[SachaG]

Yeah I think I agree. It's probably good to make the props explicit, otherwise you might wonder where that document prop is coming from. I'll need to find some time to revisit this, I haven't looked at that code in a while.