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

Support React version 0.13 #638

Closed
1 of 3 tasks
gaearon opened this issue Dec 23, 2014 · 38 comments
Closed
1 of 3 tasks

Support React version 0.13 #638

gaearon opened this issue Dec 23, 2014 · 38 comments

Comments

@gaearon
Copy link
Contributor

gaearon commented Dec 23, 2014

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
Copy link
Member

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

@gaearon Do you know when they anticipate a release?

@gaearon
Copy link
Contributor Author

gaearon commented Dec 23, 2014

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
Copy link

zpao commented Dec 24, 2014

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
Copy link
Member

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

@gurdasnijor
Copy link

I can has 0.13 support? 😄

Out here on the bleeding edge!

@agundermann
Copy link
Contributor

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 Not work with react 0.13.0-beta.1 anymore #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 Too much stuff on context #744 / Eliminate Navigation/State mixins #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 mjackson changed the title Investigate support for 0.13 Support React version 0.13 Feb 24, 2015
@mjackson
Copy link
Member

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

@gurdasnijor
Copy link

@mjackson mjackson added this to the React 0.13 milestone Feb 25, 2015
@frostney
Copy link

@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
Copy link

fczuardi commented Mar 4, 2015

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

  • it doesn't work out of the box because the modules of react-router use ES6 syntax
    • bundling with browserify and reactify won't work, even if the es6 option is setted true
    • making a copy of node_modules/react-router and running babel on it to convert to es5 was the path that worked for me

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

  • Once you have a es5 converted version of react-router, <Link /> components or transitionTo api calls on components with the mixin will change the path in your location bar, but not re-render your main component
    • the ugly workaround was to call React.unmountComponentAtNode(document.body); in the callback right before the React.render(<Handler/>, document.body);
      • this feels very wrong and maybe even defeat the purpose of using react's diff, I don't know if it might be necessary due to a bug in my code, or a bug in react-router, or in react itself. Probably in my code, I will try to create a reduced test case to share.

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

@gaearon
Copy link
Contributor Author

gaearon commented Mar 4, 2015

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
Copy link

fczuardi commented Mar 4, 2015

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
Copy link

chollier commented Mar 5, 2015

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
Copy link

chollier commented Mar 6, 2015

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
Copy link

pilwon commented Mar 11, 2015

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

@johanneslumpe
Copy link
Contributor

@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
Copy link

Should we work based on the compat branch?

@nhunzaker
Copy link
Contributor

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
Copy link
Contributor Author

gaearon commented Mar 11, 2015

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

@nhunzaker
Copy link
Contributor

@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
Copy link
Contributor Author

gaearon commented Mar 11, 2015

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
Copy link
Contributor

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

@nhunzaker
Copy link
Contributor

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
Copy link
Contributor

Related: #948

@chollier
Copy link

@johanneslumpe nevermind I also get that with 0.12

@bloodyowl
Copy link

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

@pilwon
Copy link

pilwon commented Apr 6, 2015

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
Copy link

woodb commented Apr 6, 2015

@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
Copy link

Ismael commented Apr 6, 2015

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
#638 (comment).

@pilwon
Copy link

pilwon commented Apr 6, 2015

That is a fantastic news. Thanks guys 😁👍

@thealjey
Copy link

thealjey commented Apr 6, 2015

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 🙇

@fatso83
Copy link
Contributor

fatso83 commented Apr 10, 2015

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
Copy link

woodb commented Apr 12, 2015

@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
Copy link
Contributor

fatso83 commented Apr 12, 2015

@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
Copy link

@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
Copy link
Contributor

fatso83 commented Apr 14, 2015

@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
Copy link

@gaearon should this be closed then?

@gaearon
Copy link
Contributor Author

gaearon commented Apr 27, 2015

Yep.
React Router 0.13 supports React 0.13.

@gaearon gaearon closed this as completed Apr 27, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests