Flux Instance regression from v2.13.1 to v3.x

[ericclemmons]

You may remember my gist for having a custom <FluxContainer /> in v2.13.1 that is slightly modeled after <FluxComponent render={...} /> in v3.

I updated my package.json today and got this error:

Error: fluxMixin: Could not find Flux instance. Ensure that your component has either this.context.flux or this.props.flux.

(Of course, it looked related to #85, but that issue is using v2.13.1, so it must be unrelated)

As far as I can tell, there's a discrepancy with the codebase moving from React.createClass to class X extends React.Component and assigning props/statics in a different way.

If you're not aware of this issue, let me know & I'll see about creating a test-case!

[acdlite]

A test case would be greatly appreciated! It seems like there are several context-related bugs with 3.x that haven't been resolved yet since most people (including myself) were stuck on React 0.12 / Flummox 2.x until React Router updated for 0.13 compatibility.

[ericclemmons]

Haha, me too! :) I just upgraded last night. Luckily, with v0.2.x, React v0.13 + Router works great with it still!

I'll work on a test-case after I get off today...

[acdlite]

Okay, I believe I figured it out. Flummox 0.13 uses cloneElement() instead of cloneWithProps(). It looks like there are some subtle differences regarding the order in which children are mounted, which is causing this error.

This only affects the top-level FluxComponent, which the Flux instance is not already part of context. It can be circumvented by either:

1. Using the custom render prop. This avoids the need to clone the child element.
2. Adding Flux to context manually. Obviously not ideal.

Because cloneWithProps is technically not deprecated yet, we will go back to using it for now, as it fixes this issue completely. I will cut a new release soon with this change.

In the future, though, I'm starting to think we should deprecate the automatic child wrapping functionality. Kind of a shame, since for simple use cases (e.g. a single child) it provides a nice interface. But it seems to be causing more problems than its worth. The custom render prop is much more straightforward, less magical, and gives the user more control. And IMO it's not that much worse in terms of UX.

[ericclemmons]

Oddly enough, I could've sworn I tried doing render={()=>...}} and had the same issue. I'll be able to let you know for sure in a bit!

[acdlite]

Just published 3.1.2 which fixes the existing test cases. I'll leave this issue open until you can confirm.

[ericclemmons]

Fixed it!! God bless you @acdlite! :)

[nelix]

Another idea for future API could be the based around a higher-order component that came wrap the component as seen here: https://medium.com/@dan_abramov/mixins-are-dead-long-live-higher-order-components-94a0d2f9e750

I think this looks nicer than providing render as a prop and encourages reusability.

[ericclemmons]

Based on my gist that creates a FluxContainer (which is basically what FluxComponent + render does in v3), I've adopted the following:

https://gist.github.com/ericclemmons/70b5c698f37689ce4d51

FluxApp.createContainer = function(Component, props) {
  return React.createClass({
    displayName: `${Component.displayName}Container`,

    render() {
      return (
        <FluxContainer component={Component} {...this.props} {...props} />
      );
    },
  });
};

This way, I can do export default FluxApp.createContainer(User, { ... });

I'm not sure how this can get any cleaner or be more "higher-order". The thing is, for only rendering when props are loaded, the <Wrapper><Component /></Wrapper> doesn't work (because the component has been created + rendered), and forcing everything to have a separate UserContainer or similar ends up being tons of bloat.

[acdlite]

Yeah, I'm unconvinced that a higher-order component is a better interface. But I'd accept a PR that adds it as an alternative to FluxComponent. It should be straightforward to implement using reactComponentMethods: https://github.com/acdlite/flummox/blob/master/src/addons/reactComponentMethods.js

Let's discuss that in a separate issue, though.

[nelix]

Is it possible that this issue was being caused by people not passing context to super() in their constructors?

[ericclemmons]

Maybe? I remember testing the value of context and always getting { flux: undefined }. However, between v3.1.1 & v3.1.2 I didn't make any code changes and the same code I was using from v2.13.1 finally worked!