flummox icon indicating copy to clipboard operation
flummox copied to clipboard

Improve HoC connectToStores Docs

Open burabure opened this issue 9 years ago • 24 comments

I think that the connectToStores HoC pattern is a huge plus for Flummox, and that the actual docs don't showcase how awesome it is (Easy, intuitive Relay-like data fetching from stores).

I'd be happy to send a PR with an updated Quick start, examples and a "Why HoC > FluxComponent > fluxMixin". Also I could introduce

contextTypes: {
    flux: React.PropTypes.object
  },

because without it HoC becomes very clunky.

@acdlite basically I just want to check if this is the direction that you want on the guide/docs?

burabure avatar Apr 05 '15 02:04 burabure

Yes to all, that would be a huge help

acdlite avatar Apr 06 '15 02:04 acdlite

just checking: right now the HoC needs a higher FlummoxComponent to pass the flux object to the context right?. or is there a way to pass the flux object to the HoC?

burabure avatar Apr 09 '15 23:04 burabure

Correct, currently no way to pass Flux directly to the HoC

acdlite avatar Apr 10 '15 00:04 acdlite

Slightly unrelated: are there plans to add this behavior to the HoC?, should I open an issue?

burabure avatar Apr 10 '15 00:04 burabure

Yeah, eventually. Also want to add injectActions, but I'm not sure what the best API for that is yet.

acdlite avatar Apr 10 '15 00:04 acdlite

been working on the docs!. Quickstart and React integration are ready. i'll be doing "Why HoC > FluxComponent > fluxMixin" next week!

https://github.com/acdlite/flummox/compare/master...burabure:master

burabure avatar Apr 18 '15 22:04 burabure

:+1: Documentation on what FluxComponent.connectToStores() solves, and how (notably what do its parameters represent) would be most welcome.

Once i hit the point (a very lucky shot) where "it works but i do not know why" i figured out what did what by intentionally breaking stuff, but its not exactly evident. And i was not even aware that HoC != FluxComponent.

barrystaes avatar Apr 21 '15 08:04 barrystaes

:+1:

dozoisch avatar Apr 25 '15 02:04 dozoisch

Thanks a lot for these docs @burabure ! It helped me a loooot. If I could make a comment on this part : (https://github.com/burabure/flummox/blob/master/docs/docs/guides/react-integration.md)

// Pass an object of store keys mapped to getter functions
MyComponent = connectToStores(MyComponent, {  
  posts: store => ({
    postBody: store.getPostBody(this.props.post.id),
  }),
  comments: store => ({
    comments: store.getCommentsForPost(this.props.post.id),
  })
});

Depends on your module loading system, if you're using io.js, this will be ok but in certain cases like with Babel, this could be undefined. => https://github.com/babel/babel/issues/562

I think it could be helpful for those like me to include this point in the documentation. Thoughts ?

akofman avatar Apr 28 '15 08:04 akofman

thanks @akofman, happy to help =)

about the issue: I think you're right. with ES6 arrow functions this would point to the outside scope instead of connectToStores context. thanks!

i'll fix it right away!

burabure avatar Apr 28 '15 18:04 burabure

Store state getters are not bound to the component instance, as of v3.0, so even without an arrow function, this.props will always be undefined. The way to do this now is to use the props parameter:

// Pass an object of store keys mapped to getter functions
MyComponent = connectToStores(MyComponent, {  
  posts: (store, props) => ({
    postBody: store.getPostBody(props.post.id),
  }),
  comments: (store, props) => ({
    comments: store.getCommentsForPost(props.post.id),
  })
});

acdlite avatar Apr 28 '15 18:04 acdlite

Oops, I hate that button :D

acdlite avatar Apr 28 '15 18:04 acdlite

thank you @acdlite. is that behaviour documented?, I think there's something about them in the upgrade guide, but it seems everything else is outdated.

I'll add it to the component, HoC and mixin just in case =)

burabure avatar Apr 28 '15 18:04 burabure

I don't think it's explicitly documented anywhere, just reflected in the examples.

Thanks :)

acdlite avatar Apr 28 '15 18:04 acdlite

just checking: this.props on the fluxcomponent would also be undefined since 3.0 right?

also, i think connectToStores={{ double wrapping with {} is wrong right?

class BlogPostPage extends React.Component {
  render() {
    <div>
      <SiteNavigation />
      <MainContentArea>
        <FluxComponent connectToStores={{
          posts: store => ({
            post: store.getPost(this.props.postId),
          })
        }}>
          <BlogPost />
        </FluxComponent>
      </MainContentArea>
      <SiteSidebar />
      <SiteFooter />
    </div>
  }
}

burabure avatar Apr 28 '15 19:04 burabure

In that case it refers to the props of the owner (BlogPostPage), which is probably what you want.

Double wrapping is correct because you're passing an object. The first set of braces is for embedding a JavaScript value in JSX; the second set is the object itself. It's the same concept as when you pass an inline object to the style props.

acdlite avatar Apr 28 '15 19:04 acdlite

stateGetter's second param (props) in the docs now.

https://github.com/burabure/flummox/commit/145f977adf8f6bd2028cd6231321cdbf882a33bd

@acdlite if you have a couple minutes for a quick check, in case I messed up some detail, it would be awesome =)

burabure avatar Apr 28 '15 19:04 burabure

typo fixed and proof read. ready to pull in =)

burabure avatar Apr 29 '15 17:04 burabure

Thanks! I'll try to get this merged in tonight. Sorry I've been a bit less responsive lately — I accepted a new job in a new city, so I've been really busy :)

acdlite avatar Apr 29 '15 18:04 acdlite

hope the new job and the moving stuff are getting less crazy ;D

Let me know if I can help with anything to make this PR easier to pull

burabure avatar May 16 '15 00:05 burabure

@acdlite we should get this merged I think :)

johanneslumpe avatar May 30 '15 19:05 johanneslumpe

+1 this would be helpful to have merged. I cloned vesparny's repo and it uses the HoC, and with no documentation it was pretty confusing.

swordsreversed avatar Jun 11 '15 14:06 swordsreversed

+1

vicentedealencar avatar Jun 13 '15 02:06 vicentedealencar

While the function itself is anonymous - now that connectToStores accepts actions I'd recommend renaming its usage in the docs to connectToFlux or something similar.

mmerickel avatar Jun 15 '15 16:06 mmerickel