ericclemmons / react-resolver

Async rendering & data-fetching for universal React applications.
https://ericclemmons.github.io/react-resolver
Other
1.65k stars 52 forks source link

owner-based and parent-based contexts differ #29

Closed ericclemmons closed 9 years ago

ericclemmons commented 9 years ago

On our internal project I saw:

Warning: owner-based and parent-based contexts differ (values: [object Object] vs [object Object]) for key (parent) while mounting ResolverContainer (see: http://fb.me/react-context-by-parent)

This may be related to #27, or it may be some subtle difference.

(Although, there should be several updates in React v0.14 regarding context!)

ericclemmons commented 9 years ago

Not seeing this anymore as of v1.1.6.

ericclemmons commented 9 years ago

Hmmm... it still exists:

this.resolver = new Resolver();
this.resolve = { counter: function() { return ++counter; } };
this.element = (
  <Container resolve={this.resolve} resolver={this.resolver}>
    <PropsFixtureContainer />
  </Container>
);

Warning: owner-based and parent-based contexts differ (values: [object Object] vs undefined) for key (parent) while mounting ResolverContainer (see: http://fb.me/react-context-by-parent)

Me thinks the cloneWithProps just doesn't work as expected.

Need to test out what happens when I use element or component instead of children...

ericclemmons commented 9 years ago

This can be seen from the code in #37:

$ npm test -- test/Container.lifecycle.test.js 
  <Container />
    constructor
      ✓ should not resolve yet
    Lifecycle
      .componentWillMount
Warning: owner-based and parent-based contexts differ (values: `[object Object]` vs `undefined`) for key (parent) while mounting ResolverContainer (see: http://fb.me/react-context-by-parent)
Warning: owner-based and parent-based contexts differ (values: `[object Object]` vs `undefined`) for key (resolver) while mounting ResolverContainer (see: http://fb.me/react-context-by-parent)

No idea...

BerkeleyTrue commented 9 years ago

I think the problem may lay here https://github.com/ericclemmons/react-resolver/blob/master/src/Resolver.js#L150 you are providing childContextType to ComponentContainer but the Container component is not the child of ComponentContainer. ComponentConainer is the owner of Container.

ericclemmons commented 9 years ago

I'm wondering if this is something that'll be automagically fixed with React v0.14.

The fact is, between React.cloneElement not behaving the same as cloneWithProps and child vs. owner being pretty darn ambiguous, I think I'll have to wait it out & actually solve an error, since that'll (hopefully) at least have a stack track.

BerkeleyTrue commented 9 years ago

0.14 will get rid of Owner based context and the warning will probably go away but the issue lies with not properly passing context to components not with your use of cloneWithProps. These warning are coming from ResolveContainer not the container component. That bit seems to be working. (I think changing the name of ComponentContainer to something like ComponentWrapper to differentiate it would help with the confusion).

Since you are relying heavily on context in component it is important to get at least childContext working properly for the higher lever ComponentContainer.

NVM. It seems like the issue might be in the Container component after all

ericclemmons commented 9 years ago

Yea, I can't actually nail down the warning since I have several projects using it quite successfully =/

I did notice that there were some warnings because parent (which is passed down via getChildContext) is a different object. I can only presume that React expects these context keys to be "global" and immutable, in that parent will always be the same value, no matter where it is in the tree.

What I'll likely end up doing is re-factoring (yet again) to not rely on parent and instead of Container being used for both providing and receiving context, creating a <Resolver.Context /> to supply the child context and Containers will only receive it.

The trick is trying to figure out the tree structure for caching resolver.states, which is required server-side rendering.

(As an aside, I'm not simply incrementing container IDs because, if there are 2+ branches in the tree and they resolve in different orders each time, their child containers will always end up with different IDs).

ericclemmons commented 9 years ago

Being resolved in v2. It's mostly React's context & v0.13.

punmechanic commented 9 years ago

I'm assuming this has been resolved due to the close, however I am still getting this issue when using this library in tandem with react-router.

  "dependencies": {
    "react": "^0.13.3",
    "react-resolver": "^2.0.5",
    "react-router": "^1.0.0-rc1"
  },

I don't know enough about contexts to solve this myself, but whenever I use a @resolve-decorated component inside any component that is a route with react-router, I get this warning and my code does not function correctly (I'll get a noscript element instead of the expected h1).

Posting this here instead of react-router as this is the library that appears to cause the actual issue. Sample code:

class HomePage extends Component {
  render() {
    return <section>
      <Wrapped /> // This component causes the issue. Even if this is the direct child we get the same warning
    </section>;
  }
}

@resolve('foo', function(props) {
  return new Promise(function(resolve) {
    resolve(12345);
  });
})
class Wrapped extends Component {
  render() {
    return <h1>{this.props.foo}</h1>;
  }
}

export default <Route>
  <Route path='/:id' component={HomePage}/>
</Route>;

Resolving the @resolve decorator resolves (puns) the issue, but I unfortunately then lose the ability to resolve data ahead of time on the server-side, which sucks.

ericclemmons commented 9 years ago

Hmmm, I wonder if it's the tandem use of React Router v1 & React v0.13.

For example, I haven't seen this in either one of my examples since the re-write:

https://github.com/ericclemmons/react-resolver/tree/master/examples/react-v0.13 https://github.com/ericclemmons/react-resolver/tree/master/examples/react-v0.14

I've also been waiting for React Router v1 to officially launch since much of the code has been in flux during the betas.

Can you show the server-rendering part of your code, though?

punmechanic commented 9 years ago

@ericclemmons certainly, it isn't like your examples though, unfortunately, i couldn't get them working (perhaps I'm doing it wrong; react-router and react-resolver are both very new libraries to me, so I probably shouldn't be trying to kill one bird with two stones). It looks something along the lines of this (I literally just did a git reset --hard...)

const RoutingContext = React.createFactory(Router.RoutingContext);
Router.match({ location: location, routes: routes }, function(error, redirectLocation, routerProps) {
  Resolver.resolve(function() {
    return RoutingContext(routerProps);
  }).then(function(context) {
    const Resolved = React.createFactory(context.Resolved);
    const data = context.data;

    const rendered = React.renderToString(Resolved);
    console.log(rendered); // or potentially res.send(rendered);
  });
});

Rough approximation above. I'm trying to avoid JSX in the server side code, the resolved application itself is pulled from another npm package which is transpiled (this shouldn't make a difference). I appreciate this isn't how you've set it up to be exactly but I would have expected this to work (at least on the server). Right now, I'm not concerned about client-side performance as I have that down, I mainly just want to get the isomorphic data fetching working so I can get on with it.

Feel free to throw the book at me for not following your recommendations.

ericclemmons commented 9 years ago

Shoot, it looks like I need to try out RC1. Apparently the <Router history={...}> thing is in the past!

ericclemmons commented 9 years ago

Should it be return React.createElement(RoutingContext, routerProps)?

http://babeljs.io/repl/#?experimental=true&evaluate=true&loose=false&spec=false&playground=false&code=import%20%7B%20Resolver%20%7D%20from%20%22react-resolver%22%3B%0A%0Aconst%20props%20%3D%20%7B%7D%3B%0A%0AResolver.resolve(()%20%3D%3E%20%3CRoutingContext%20%7B...props%7D%20%2F%3E)%3B

punmechanic commented 9 years ago

@ericclemmons, React.createFactory(RoutingContext)(routerProps) === React.createElement(RoutingContext, routerProps), at least functionally, surely?

(although yes, perhaps it should be the latter)

ericclemmons commented 9 years ago

No idea! Just babel-ifying things.

punmechanic commented 9 years ago

The direct equiv would be what you suggested, but as far as I am aware createFactory has the exact same functionality to a degree, at least in this context

ericclemmons commented 9 years ago

Oh, also, the context stuff in React v0.13 is a PITA. We're still running v0.13 & RR v0.13 in production, but see the warning (although stuff still works) because we have code changing context like:

<SomeContextProvidingComponent>
  <MyResolverComponentThatAlreadyHasContext />
</SomeContextProvidingComponent>
punmechanic commented 9 years ago

See, the problem is not even the warning, its that functionality actually breaks otherwise I would jsut ignore the warning. I just did npm i react, so I've no particular ties to this version of react if downgrading might help.

ericclemmons commented 9 years ago

Oh wowsers! And, actually, if you have no particular ties to a version, I'd say try out the v0.14:

http://facebook.github.io/react/blog/2015/09/10/react-v0.14-rc1.html

There are some warnings, but it fixes the context issue.

All in all, I think what I need to do is test out the RR v1-rc1 that you're using. They've changed the API quite a bit for it and I haven't used it yet.

I'm normally all for using edge releases, but good God this ecosystem changes a lot :D