remix-run / react-router

Declarative routing for React
https://reactrouter.com
MIT License
53.03k stars 10.31k forks source link

Support React version 0.13 #638

Closed gaearon closed 9 years ago

gaearon commented 9 years ago

React 0.13 brings parent-based context instead of owner-based context.

I'm getting tons of warnings with just published react@0.13.0-alpha.1 and the main problem seems to be that context is defined on router (makePath, makeHref, transitionTo, replaceWith, etc) but we're giving RouteHandler in render, so these keys don't appear in RouteHandler's parent-based context.

I still can't think quite straight about these changes but I assume some kind of wrapping RouteHandler into context parent will be necessary.

Edit (@mjackson): let's keep a checklist here:

mjackson commented 9 years ago

Ooo ... exciting! I need to find some time to look into this.

@gaearon Do you know when they anticipate a release?

gaearon commented 9 years ago

Usually betas come in a week or two before the release, but I don't know for certain, and this one is explicitly work in progress as stated by @zpao.

The good thing is, following React's deprecation policy, everything still works (but spits out a lot of warnings). There also seems to be a bug in warnings themselves: instead of being printed once, they seem to be printed for every child (which explains why there's such a load of them).

zpao commented 9 years ago

Early next year. Hopefully around the conf, but I refuse to make promises :)

I think 0.13 won't actually have parent context as the default and it might not actually be exposed at all except via the warning. Since we really don't know how people are making use of context in the wild, I'm not sure if we can just go (maybe) break them, even if we never promised support. But we can help with the transition for when we know it would break, thus the warning.

mjackson commented 9 years ago

HEY EVERYONE @zpao SAID THAT REACT 0.13 IS GONNA BE RELEASED IN JANUARY

gurdasnijor commented 9 years ago

I can has 0.13 support? :smile:

Out here on the bleeding edge!

agundermann commented 9 years ago

Just did some testing and here's the issues I've found thus far:

  1. createClass static methods aren't automatically bound anymore Causes an error in router.handleLocationChange, since the method is passed without .bind to Locations. Could be a bug in react, not sure. See #818. Also affects transition hooks relying on this, which includes our test handlers.
  2. createClass().type is deprecated In Routing.js, we simply have to replace checks like element.type === Redirect.type with element.type === Redirect. Not backwards-compatible, though.
  3. Context changes The way I understand it, this isn't even a parent vs owner context problem. In order to support and compare the results of the two context mechanisms, react calls getChildContext twice. Since we're doing stuff like .bind and .concat, the identity checks will fail and the warnings will show up. I guess this is best fixed with #744 / #835 (only put router object on context).
  4. React.render callback not always called The render callback isn't called instantly on re-renders, but only after doing another render. Seems like a bug in react. Causes a lot of tests relying on that to time out.
mjackson commented 9 years ago

Just FYI, I'm working on 0.13 support in the 0.13-compat branch

gurdasnijor commented 9 years ago

Timing couldn't be better :)

http://facebook.github.io/react/blog/2015/02/24/react-v0.13-rc1.html

frostney commented 9 years ago

@mjackson Thanks for the compat branch. I'm having some issues there. The initial/default route works fine, but when I try to change to another route through the URL, I'm getting:

react-router.js:1603 Uncaught TypeError: Cannot read property 'dispatch' of undefined
react-router.js:1603 handleLocationChange
react-router.js:445 (anonymous function)
react-router.js:444 notifyChange
react-router.js:457 onHashChange

And nothing else happens.

I'm pretty new to React and react-router, but if anyone could give me some pointers, I'd like to take a crack at it.

fczuardi commented 9 years ago

My experience so far in trying to make rack/react-router#0.13-compat work with react@0.13.0-rc2 :

entering the folder and running npm run build-npm, and using require('react-router/build/npm') fixed :)

I found the bug, my problem was with the PureRenderMixin…

gaearon commented 9 years ago

making a copy of node_modules/react-router and running babel on it to convert to es5 was the path that worked for me

There's a build step to do this, happens before publishing to NPM.

fczuardi commented 9 years ago

Oh, I see it, https://github.com/rackt/react-router/blob/master/package.json#L14, npm run build-npm should work then, I will give it a try.

chollier commented 9 years ago

I get some :

Warning: owner-based and parent-based contexts differ (values: `[object Object],[object Object]` vs `[object Object],[object Object]`) for key (routeHandlers) while mounting RouteHandler (see: http://fb.me/react-context-by-parent)
chollier commented 9 years ago

Which doesn't seem a problem, however when opening a new tab and hitting a route, I get :

Uncaught ReferenceError: _currentRoute is not defined         Route.js:52

but when refreshing the page it goes right.

pilwon commented 9 years ago

v0.13 has just been released: http://facebook.github.io/react/blog/2015/03/10/react-v0.13.html

johanneslumpe commented 9 years ago

@chollier That actually seems weird to me, because _currentRoute is a global inside the module, it should always be defined.

Just tried to run the test of the compat branch with react 0.13 - tons of errors. Not sure where to start fixing them. Tried to add contextTypes to the nested test component, just to see if that helps with the owner/parent context mismatch, because the nested component renders the RouteHandler, which requires those context props. But no luck with that.

@ryanflorence @mjackson any pointers where exactly help is required and what currently is already being worked on to fix the compat issues?

daviferreira commented 9 years ago

Should we work based on the compat branch?

nhunzaker commented 9 years ago

I noticed in this branch that the 0.13 class syntax is being used. Does this mean that the 0.13-compat branch will not be backwards compatible?

Edit: This is really more a question of how to address the 0.13 issues, rather than maintaining backwards compatability

gaearon commented 9 years ago

Router can still be used both in new and old style components. This is not changing.

nhunzaker commented 9 years ago

@gaearon Yes, but this branch defines the Route components with the 0.13 class syntax. Unless I am mistaken, this eliminates 0.12 support.

gaearon commented 9 years ago

Oh, sorry, I misunderstood your question. When React bumps major version, next version of RR comes out requiring it. So yes, React Router 0.13 won't support React 0.12, as far as I know.

nhunzaker commented 9 years ago

@gaearon :+1: That'll make the Component.type issue quite a bit easier to address

nhunzaker commented 9 years ago

I think I fixed 0.13, but I don't really know yet. Would anyone be willing to verify my 0.13.0 branch?

https://github.com/rackt/react-router/compare/rackt:0.13-compat...nhunzaker:0.13.0?expand=1

nhunzaker commented 9 years ago

Related: https://github.com/rackt/react-router/pull/948

chollier commented 9 years ago

@johanneslumpe nevermind I also get that with 0.12

bloodyowl commented 9 years ago

I created a PR to fix the owner vs parent context here : #959

pilwon commented 9 years ago

I'm just curious because it's been about a month since React v0.13 was released and almost half a year since this issue was originally created...

Is anyone actually working on this or are we all just waiting for someone else to take on this challenge and solve the problem?

woodb commented 9 years ago

@pilwon I'm pretty sure that this already supports React 0.13, I'm not sure why it's still open... Following the upgrade guide made it pretty easy to make the changes necessary for React 0.13.

Ismael commented 9 years ago

The issue hasn't been updated, but the latest version supports react 0.13. On Apr 6, 2015 9:41 AM, "Pilwon Huh" notifications@github.com wrote:

I'm just curious because it's been about a month since React v0.13 was released and almost half a year since this issue was originally created. Is someone actually working on this or are we all just waiting for someone to take on this challenge and solve the problem?

— Reply to this email directly or view it on GitHub https://github.com/rackt/react-router/issues/638#issuecomment-90045666.

pilwon commented 9 years ago

That is a fantastic news. Thanks guys :grin::+1:

thealjey commented 9 years ago

This has been fixed a while ago actually, tested with react 13.1, and it's pretty great to say the least. The best extension for the ultimate framework! Thanks for this, and I bow down before your awesome smartitude :bow:

fatso83 commented 9 years ago

The warnings introduced with the 0.12->0.13 upgrade still appear, so until that is sorted out, I cannot see how you can say that this issue has been fixed, @thealjey and @woodb

Warning: owner-based and parent-based contexts differ (values:[object Object]vs[object Object]) for key (routeHandlers) while mounting RouteHandler and Warning: DefaultRoute.type is deprecated. Use DefaultRoute directly to access the class.

Do agree that it is functionally working, though. "react": "0.13.1", "react-router": "0.13.2"

woodb commented 9 years ago

@fatso83 have you gone through the migration guide?

We're using react 0.13.1, react-router 0.13.2 and node v0.12.1 and are not receiving any warnings, though we were before going through the guide...

fatso83 commented 9 years ago

@woodb Yeah, as it's public, you can see that I follow the upgrade guide pretty close. Not really doing any fanciness (no Mixins), just basic route setup, one sub-level using RouteHandler and router.run(). So not sure why I get these warnings.

I am not seeing any warnings when NODE_ENV=production, but that is to be expected.

deakster commented 9 years ago

@fatso83 In dev mode you are using your build/jslib/react-router.js file. I don't believe that's react-router 0.13.2, since replacing that with the actual react-router 0.13.2 gets rid of the warnings for me.

fatso83 commented 9 years ago

@deakster Thanks for checking that. Issues gone. I'll gladly shut up and never utter a word on GH again... No clue what version of react-router that was either. Maybe that could be a clue to add the version info at build time like React does? Think I'll go hide under a rock for a few weeks now.

saulshanabrook commented 9 years ago

@gaearon should this be closed then?

gaearon commented 9 years ago

Yep. React Router 0.13 supports React 0.13.