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

Improve HoC connectToStores Docs #136

Open
burabure opened this issue Apr 5, 2015 · 24 comments
Open

Improve HoC connectToStores Docs #136

burabure opened this issue Apr 5, 2015 · 24 comments
Labels

Comments

@burabure
Copy link

burabure commented Apr 5, 2015

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?

@acdlite
Copy link
Owner

acdlite commented Apr 6, 2015

Yes to all, that would be a huge help

@burabure
Copy link
Author

burabure commented Apr 9, 2015

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?

@acdlite
Copy link
Owner

acdlite commented Apr 10, 2015

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

@burabure
Copy link
Author

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

@acdlite
Copy link
Owner

acdlite commented Apr 10, 2015

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

@burabure
Copy link
Author

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

master...burabure:master

@barrystaes
Copy link

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

@dozoisch
Copy link

👍

@akofman
Copy link

akofman commented Apr 28, 2015

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.
=> babel/babel#562

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

@burabure
Copy link
Author

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!

@acdlite
Copy link
Owner

acdlite commented Apr 28, 2015

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 acdlite closed this as completed Apr 28, 2015
@acdlite acdlite reopened this Apr 28, 2015
@acdlite
Copy link
Owner

acdlite commented Apr 28, 2015

Oops, I hate that button :D

@burabure
Copy link
Author

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 =)

@acdlite
Copy link
Owner

acdlite commented Apr 28, 2015

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

Thanks :)

@burabure
Copy link
Author

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

@acdlite
Copy link
Owner

acdlite commented Apr 28, 2015

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.

@burabure
Copy link
Author

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

burabure@145f977

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

@burabure
Copy link
Author

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

@acdlite
Copy link
Owner

acdlite commented Apr 29, 2015

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 :)

@burabure
Copy link
Author

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

@johanneslumpe
Copy link
Collaborator

@acdlite we should get this merged I think :)

@swordsreversed
Copy link

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

@vicentedealencar
Copy link

+1

@mmerickel
Copy link

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.

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

No branches or pull requests

10 participants