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

Prepare for React v0.13+ #8

Merged
merged 57 commits into from
Apr 13, 2015
Merged

Prepare for React v0.13+ #8

merged 57 commits into from
Apr 13, 2015

Conversation

ericclemmons
Copy link
Owner

Sebastian Markbåge wrote about Immutable Props here:

http://facebook.github.io/react/blog/2015/02/24/streamlining-react-elements.html#immutable-props

react-resolver does a "double mount/render": once componentWillMount to initially fire off a promise for data dependencies, then for the second pass to fill in the gaps in props:

if (promise.isFulfilled()) {
assign(this.props, promise.value());

This technique is used because this.setProps and this.replaceProps can only be down on root-level components.

The question is, will this pose any issues dynamic prop resolution with 0.13?

/ping @sebmarkbage

@ericclemmons ericclemmons self-assigned this Feb 24, 2015
@sebmarkbage
Copy link

Yes, this pattern is problematic. Even in 0.12, because a consumer of this component might expect this to remain a promise and you're changing it from under them.

I would recommend that you implement this as a higher-order component. That way you have a layer where you are in full control over all the props without risk colliding with other mixins etc.

https://gist.github.com/sebmarkbage/ef0bf1f338a7182b6775

It will also work for any kind of React component. Including ES6 classes.

@ericclemmons
Copy link
Owner Author

Well, it doesn't really change it out from underneath them.

This enables the possibility that:
<MyLazyComponent user={explicitValue} /> is lazily created via statics.resolve.user returning a Promise or calling done. (In the presence of existing props (e.g. user), no work is done, so there's no clashing.)

Is the only way to really do this in the future will be "wrapper" components, since mixins is going by the wayside?

In which case, will this mean that every component that wants to lazy-load data should use a wrapper?

(It may be worth confirming or denying if this is how Relay is injecting props, since we have the exact same method of solving this problem :D)

P.S. Thanks for joining the discussion @sebmarkbage!

@sebmarkbage
Copy link

That props object is the same object as the one in the parent's element. So this (somewhat contrived) pattern would break:

var constantElement = <MyLazyComponent user={loadSomePromise()} />;
var Foo = React.createClass({

  handleClick: function() {
    constantElement.props.user.then(() => doSomething()); // Throws since this is not a promise.
  },

  render: function() {
    return <div onClick={this.handleClick}>{constantElement}</div>;
  }

});

This is just illustrative but we've had many related problems with this in the past. Relay tried to clone and replace the props object leading to a bunch of complexities and broken things.

Relay just switched to using wrappers instead. Simplifies things quite a bit.

Note that it is not true that every component that wants to lazy-load data should use a wrapper. You can use life-cycles to update your own state. That's fine.

Every time you want to pretend that you're props are something different than was passed in, you have to use a wrapper.

cc @yungsters @cpojer @josephsavona

@ericclemmons
Copy link
Owner Author

@sebmarkbage I see, so the take-away is:

Every time you want to pretend that you're props are something different than was passed in, you have to use a wrapper.

To clarify, though, this project does not override any existing props. If for some (ridiculous) reason, you specify & use a user Promise as a prop, go for it.

(See the contacts example)

You can test <Home contacts={[ ... ]} />, or lazy-load this.props.contacts (Remember, only the fulfilled value is ever assigned!) in the absence of it when creating the element: <Home />.

I just wanted to ensure you don't think I'm doing some stupid with this project. I certainly hope that Relay "fills in gaps in this.props" like this does, which makes components extremely testable!

@sebmarkbage Thanks again for the feedback and confirmation that Relay uses a wrapper to solve this same problem. This means my own & others' projects won't be left in the lurch when React v0.13+ gets released!

To those following, I'll begin a feature branch for v0.13 support to use wrappers instead of Resolver.mixin.

@ericclemmons
Copy link
Owner Author

@ericclemmons
Copy link
Owner Author

Lots to work with here:

  • Mutating props after an element is created is deprecated and will cause warnings in development mode; future versions of React will incorporate performance optimizations assuming that props aren't mutated
  • Access to most internal properties has been completely removed, including this._pendingState and this._rootNodeID.
  • Support for using ES6 classes to build React components; see the v0.13.0 beta 1 notes for details
  • Some methods that are available on createClass-based components are removed or deprecated from ES6 classes (for example, getDOMNode, setProps, replaceState).

@jeffbski
Copy link

jeffbski commented Mar 7, 2015

@ericclemmons From looking at the way you have it implemented now, if you just switch to setting the values from the promise as state instead of props then you should be fine (and of course the render method will use state instead of props), you will no longer be mutating props.

@ericclemmons
Copy link
Owner Author

@jeffbski: Good point! That may be the middle-ground for v0.2.x to work with React v0.13, but I dare say I'll have to rewrite with context or another solution, as I hear Relay is using, to solve it for both versions =/

@cpojer
Copy link

cpojer commented Mar 8, 2015

@ericclemmons Relay works with the same on 0.12 and 0.13. We are not using context for data, only for the route and that's only on the containers. To the user, the route is passed in as a prop from the container to the inner component; the fact that behind the scenes containers use context for the route is transparent to the user.

@ericclemmons
Copy link
Owner Author

@cpojer I'd love to know a bit more about the architecture on a smaller scale. The last I heard was that contexts were used, but that can mean most anything, especially when now there's mention of a (1) route, (2) container, (3) inner component and (4) context :)

If you're up for discussing more, I can be found via http://reactiflux.slack.com/, or eric@smarterspam.com.

Also, this project was originally inspired by ui-router before Relay was released, but clearly serves similar goals!

@cpojer
Copy link

cpojer commented Mar 8, 2015

I see. On a small scale, Relay containers are very simple:

class MyComponent extends React.Component {
  render() { return <div />; }
}

var MyContainer = createContainer(MyComponent);

This wraps 'MyComponent' (the 'inner component') in another component that we call a RelayContainer. Relay containers are responsible for getting the requested data into the inner component via props. We have a root component that injects the current route as context and containers have contextTypes: {route: …} set up. Containers pass the route as prop into the inner component, just like this: <InnerComponent route={this.context.route} />.

It is pretty much exactly like @sebmarkbage's example in his first comment, except of course containers do all the heavy lifting and interfacing with the internals of Relay :)

@ericclemmons
Copy link
Owner Author

@cpojer: Thank you so much for this very clear example!

@jeffbski
Copy link

jeffbski commented Mar 8, 2015

So @cpojer your above example in more detail might look like the following?

const MyComponentClassProps = { // applied to class
  contextTypes: {
    route: React.PropTypes.object.isRequired
  }
};
class MyComponent extends React.Component {
  render() { return <InnerComponent route={this.context.route} />; }
}
Object.assign(MyComponent, MyComponentClassProps);

var MyContainer = createContainer(MyComponent);

Just trying to make sure I understand the above discussion.

Thanks.

@jeffbski
Copy link

jeffbski commented Mar 8, 2015

After writing that, I realized that you are probably abstracting all the context stuff in the higher order wrapper so that data is simply coming in props.

class MyComponent extends React.Component {
  render() { return <InnerComponent route={this.props.route} />; }
}

var MyContainer = createContainer(MyComponent);

That cleans things up if you handle all that in the container wrapper.

@cpojer

@cpojer
Copy link

cpojer commented Mar 8, 2015

@jeffbski no, you essentially implemented the container above (edit: saw your second comment, yes, you are correct :) ).

Here let's take a look at this:

// container implementation
function createContainer(InnerComponent) {
  const ContainerStatics = { // applied to class
    contextTypes: {
      route: React.PropTypes.object.isRequired
    }
  };
  class Container extends React.Component {
    // do lots of magic in lifecycle methods
    render() { return <InnerComponent route={this.context.route} />; }
  }
  Object.assign(Container, ContainerStatics);
  return Container;
}

// usage:
class MyComponent extends React.Component {
  render() { return <div>{this.props.route.name}</div>; }
}

MyComponent.propTypes = {
  route: React.PropTypes.instanceOf(Route),
};

var MyContainer = createContainer(MyComponent);

There is really nothing special about how containers work. Most mixins can be modeled as containers, it is really just wrapping one component with another, in this case the container is dynamic and kinda implicit (one container per createContainer call gets created) instead of explicit wrapping of your components in render + cloning which you are probably more used to. Consider this example:

// one-of implementation
class MyComponent extends React.Component {
  render() { return <div>{this.props.route.name}</div>; }
}

class MyComponentContainer extends React.Component {
    render() { return React.cloneElement(React.Children.onlyChild(this.props.children), {
      // props to inject to the inner component
    }); }
}

// usage
class MyView extends React.Component {
  render() {
    return (
      <MyComponentContainer>
        <MyComponent />
      </MyCompontentContainer>
    );
  }
}

Now with this approach you'll have to use cloneElement but you also need to explicitly specify both the container and the inner component at every call site. The MyComponentContainer also needs to receive the configuration every single time – assume that in Relay createContainer has a second argument config that allows to specify the queries for Relay components. With this second example you'd have to pass it around all the time. That sucks and will only work for simple containers/wrappers that don't have configuration. One such example that I recently converted from a mixin to a container was something called "TapTarget" which essentially just adds a selected state between touchstart and touchend – a perfectly fine example for traditional composition with React.

@josephsavona
Copy link

@ericclemmons we'll talk about Relay's architecture a bit more in some upcoming blog posts :-)

@ericclemmons
Copy link
Owner Author

@josephsavona Sounds good! This is a pattern that, in the meantime, helped move our data needs from Handlers/Controllers to Components, so development will continue.

I just want to ensure that myself & others will have a smooth transition path should Relay be a better solution for those of us not using GraphQL (yet :D).

Thanks!

@josephsavona
Copy link

@ericclemmons of course! not suggesting otherwise, this is a helpful pattern 👍

@jeffbski
Copy link

@cpojer Thanks for the wonderful explanation. That helps tremendously.

@ericclemmons
Copy link
Owner Author

Alright, I got a functional version of this ready to test! I'll be committing momentarily...

@ericclemmons
Copy link
Owner Author

Whew boy! d748508 was a fun one to put in. I initially thought I'd only track resolved data via Resolver.createContainer, but it's much better to track each <Container />, since that's how context is maintained.

Plus, this is a requirement to ensure that the 1st render on the server loads all of the data, and the 2nd render uses the cached data for a synchronous React.renderToString.

Still, there's some more to do to ensure that deeply-nested containers track & resolve as expected...

@ericclemmons
Copy link
Owner Author

Sweet, so this is pretty much ready for release! A couple little nit-picks:

  • Client-side rendering doesn't need to store IDs or save state for serialization. state should be disposable within the resolve function.
  • The only difference between server & client is that the server needs to save state between renders (as noted above).
  • Maybe instead of deliberately setting freeze on the resolver, the renderToString and the renderToStaticMarkup can use the state object (via new Resolver(state)) as a reference for saving. If it exists, save state. If it doesn't, don't save it.
  • Some of the logic (such as getContainerState and getContainerId) seem like something better suited for the <Container />, not the Resolver.
  • setState should be chained via .then, not a "callback". This is an internal library, so no need to switch styles.

@ericclemmons
Copy link
Owner Author

Forgot a couple things:

  • Containers should support contexts (want to avoid childContextTypes and contextTypes) for defining things such as { router: React.PropTypes.function }, which would be available for any child containers' resolve functions (e.g. function userResolver(props, context) { ... })
  • Relying on new Resolver(state) doesn't make sense, since this is how the client will bootstrap it's data. Bootstrapping server-side data on the client and indicating caching doesn't make sense. Perhaps an internal CachedResolver or IsoResolver is the correct path, since all it takes to utilize the cache is defining the getContainerState method.

@ericclemmons
Copy link
Owner Author

Alright, I'm going to merge this guy down, finish off some docs, then tag & release!

Thanks for the help from everyone!

Mucho <3!

ericclemmons added a commit that referenced this pull request Apr 13, 2015
@ericclemmons ericclemmons merged commit cd5659a into master Apr 13, 2015
@iamdustan
Copy link
Contributor

👏

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

Successfully merging this pull request may close these issues.

8 participants