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

Do not pass flux prop to components wrapped with connect #251

Open
johanneslumpe opened this issue Jun 14, 2015 · 6 comments
Open

Do not pass flux prop to components wrapped with connect #251

johanneslumpe opened this issue Jun 14, 2015 · 6 comments
Labels
Milestone

Comments

@johanneslumpe
Copy link
Collaborator

We removed the flux prop for the FluxComponent but connect still passes it down. We should remove it.

@johanneslumpe johanneslumpe added this to the 4.0 milestone Jun 14, 2015
@bdowning
Copy link

How would one get access to the flux instance in the base component then? Sometimes you want it and you've removed the fluxMixin that used to put it on this.flux. FluxComponent has the render override that gets flux that you could use to customize how the wrapped component is rendered, but that doesn't make sense for connect, which is probably often going to be used as a decorator. I mean sure you could manually pull it out of context but that seems less than optimal.

Also the comment in FluxComponent.js still says it injects flux into the children's props, so that should be fixed if it's no longer the case...

@bdowning
Copy link

(Actually reading the code I'm not sure I see where flux is injected into connect's base component anyway, so this may not be a change in behavior at all...)

@johanneslumpe
Copy link
Collaborator Author

@bdowning FluxComponent does not pass flux to the render method anymore. If you check here you will see that the flux prop is excluded from the props that get passed to render or to a wrapped child.

You're right about the comment, I'll get that fixed!

As you said, the solution is to pull it from context if you really require it. By being able to inject actions into your components now, I'm not sure in which case you would really want it.

As for connect, the flux prop gets injected here because we are just spreading all props to the wrapped component.

@bdowning
Copy link

But how does flux get on the props of the connect wrapper component to be passed to the base component on the line you mention? The connect wrapper calls this.initialize which sets this.flux on the wrapper component, but I'm not seeing anywhere it assigns flux to its props. It looks to me like flux will be passed to the base component if and only if it was passed as a prop to the wrapper. Seems like if it's pulled from context instead (as is probably typically the case) it will be absent.

@johanneslumpe
Copy link
Collaborator Author

@bdowning I noticed this because I have a connected component as a root component - which receives the flux prop because I'm passing it in. We should be consistent here. If the FluxComponent does not pass it to its children, then connect should not pass it to the base component either.

@bdowning
Copy link

FluxComponent is clearly visually a wrapper, so having it throw away props males sense. But when class Foo uses connect as a decorator (which sort of implies a fancy class even though it is a HoC under the hood) I think its a bit surprising that some props that you pass to Foo don't actually make it there.

But then it also doesn't get wrapped statics currently either, so it's hardly transparent.

Personally I'm not sold on the use-HoCs-for-all-the-things mentality and am sort of sad that mixins have fallen out of favor (and fluxMixin in particular has gone away, even though createClass has not) but I acknowledge that I seem to be in the minority here. (Then again I like Common Lisp and CLOS, who's predecessor arguably created the concept in the first place! :). Certainly, though, working with react-router seemed to be a lot easier and more consistent with mixins than with HoCs. But I can always maintain my own fluxMixin if I get desperate.

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

3 participants