Skip to content

Upgrade to React 0.12 #97

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

Closed
jamwaffles opened this issue Oct 29, 2014 · 20 comments
Closed

Upgrade to React 0.12 #97

jamwaffles opened this issue Oct 29, 2014 · 20 comments

Comments

@jamwaffles
Copy link

More info in their blog post here.

Quite a few breaking changes. Not sure how this is going to work with regards to backwards compatibility.

@STRML
Copy link
Owner

STRML commented Oct 29, 2014

Another big release - I’ll be handling the update shortly as I update my personal projects, which use RRC.

On Oct 29, 2014, at 8:33 AM, James Waples [email protected] wrote:

More info in their blog post here http://facebook.github.io/react/blog/2014/10/28/react-v0.12.html.

Quite a few breaking changes. Not sure how this is going to work with regards to backwards compatibility.


Reply to this email directly or view it on GitHub #97.

@STRML
Copy link
Owner

STRML commented Oct 29, 2014

Re: Backcompat, we’ve traditionally told users stuck on older versions of React to use older versions of RRC. Unfortunately, with the major breaking changes we’re seeing often in React, that is often the only way to do it.

On Oct 29, 2014, at 8:33 AM, James Waples [email protected] wrote:

More info in their blog post here http://facebook.github.io/react/blog/2014/10/28/react-v0.12.html.

Quite a few breaking changes. Not sure how this is going to work with regards to backwards compatibility.


Reply to this email directly or view it on GitHub #97.

@jamwaffles
Copy link
Author

Noted, thanks for the quick response.

@STRML
Copy link
Owner

STRML commented Oct 29, 2014

Just to update - this is pretty involved. The 0.12 hack that calls React.createElement transparently within JSX breaks Route and Location, since they are simply wrapper functions. Turning them into elements is not a perfect solution either:

  • It changes the behavior of refs - refs are now bound to the creating component (say, App), not the router. This is actually more consistent but breaks backcompat.
  • It breaks context - since the element is actually being rendered by the App, and not by the Router, the router is no longer brought along as context.
    • Maybe we remove the use of context entirely (after all it is very much subject to change). We then pass router to components rendered by the router, and require that contextual routers receive a parent router instance.

I'm working on this on the react-0.12 branch, which I'll squash when done.

STRML added a commit that referenced this issue Oct 29, 2014
@jamwaffles
Copy link
Author

Hmm. Reading the example in docs/index.jsx doesn't look too bad. The LOC would still be the same, albeit with a bit more complexity. I haven't looked at how React works internally so I can't offer much more than that.

STRML added a commit that referenced this issue Oct 29, 2014
@STRML
Copy link
Owner

STRML commented Oct 29, 2014

The react-0.12 branch is now passing all tests. Could you please try it with your application, if you've updated to 0.12?

@jamwaffles
Copy link
Author

I had a good stab at getting RRC working with 0.12 but ran into a few issues. I get a lot of warnings about components calling a React component directly (this definitely occurs with <Link>s, perhaps other stuff) and for some reason I can't use the nice {...props} spread operator, but that's probably something to do with my build.

Additionally, client side routing appears to be broken with an error complaining about either refs being undefined, or a key on refs being undefined.

Apologies for the vague report. I'll get some more details together for whatever you need if you let me know.

@STRML
Copy link
Owner

STRML commented Oct 30, 2014

Is that with the new branch?
On Oct 30, 2014 10:12 AM, "James Waples" [email protected] wrote:

I had a good stab at getting RRC working with 0.12 but ran into a few
issues. I get a lot of warnings about components calling a React
component directly (this definitely occurs with s, perhaps other
stuff) and for some reason I can't use the nice {...props} spread
operator, but that's probably something to do with my build.

Additionally, client side routing appears to be broken with an error
complaining about either refs being undefined, or a key on refs being
undefined.

Apologies for the vague report. I'll get some more details together for
whatever you need if you let me know.


Reply to this email directly or view it on GitHub
#97 (comment)
.

@jamwaffles
Copy link
Author

Yes it is. I installed it through NPM using the Github repo URL ending in #react-0.12 to select the branch.

@jtuchsen
Copy link

jtuchsen commented Nov 1, 2014

I think I figured out the issue with the warnings. React depreciated calling components directly to create them. Instead they now want you to wrap them in a factory method, which JSX does for you automatically but you need to do manually when not using it. So the handler components passed into the router need to be wrapped. I created a pull request to this effect, but I'm not sure if calling the factory method as late as possible is correct. I just started using the React framework, so sorry if I'm completely off base with this :).

@jamwaffles
Copy link
Author

I've managed to integrate React 0.12 into my app. It works fine but there are quite a few warnings for me as well. These are my findings:

Versions:

$ npm -v
1.4.28

$ node -v
v0.10.33

Package versions used:

{
    ...
    "react": "^0.12.0",
    "react-async": "^2.0.1",
    "react-router-component": "git+https://github.com/STRML/react-router-component.git#react-0.12",
    "reactify": "^0.15.2"
    ...
}

When a route doesn't have a handler:

TypeError: Cannot read property 'ref' of null
    at RouteRenderingMixin.renderRouteHandler (/Users/jameswaples/Repositories/Telegraph/node_modules/react-router-component/lib/RouteRenderingMixin.js:12:64)
    at React.createClass.render (/Users/jameswaples/Repositories/Telegraph/node_modules/react-router-component/lib/Router.js:34:26)
    at ReactCompositeComponentMixin._renderValidatedComponent (/Users/jameswaples/Repositories/Telegraph/node_modules/react-router-component/node_modules/react/lib/ReactCompositeComponent.js:1260:34)
    at wrapper [as _renderValidatedComponent] (/Users/jameswaples/Repositories/Telegraph/node_modules/react-router-component/node_modules/react/lib/ReactPerf.js:50:21)
    at ReactCompositeComponentMixin.mountComponent (/Users/jameswaples/Repositories/Telegraph/node_modules/react-router-component/node_modules/react/lib/ReactCompositeComponent.js:802:14)
    at wrapper [as mountComponent] (/Users/jameswaples/Repositories/Telegraph/node_modules/react-router-component/node_modules/react/lib/ReactPerf.js:50:21)
    at ReactCompositeComponentMixin.mountComponent (/Users/jameswaples/Repositories/Telegraph/node_modules/react/lib/ReactCompositeComponent.js:808:44)
    at wrapper [as mountComponent] (/Users/jameswaples/Repositories/Telegraph/node_modules/react/lib/ReactPerf.js:50:21)
    at ReactDOMComponent.ReactMultiChild.Mixin.mountChildren (/Users/jameswaples/Repositories/Telegraph/node_modules/react/lib/ReactMultiChild.js:195:42)
    at ReactDOMComponent.Mixin._createContentMarkup (/Users/jameswaples/Repositories/Telegraph/node_modules/react/lib/ReactDOMComponent.js:260:32)

Trying to use JSX splats:

TypeError: Object function Object() { [native code] } has no method 'assign'

Is this because I'm not using Node 0.11.x? Probably.


This code:

var Dashboard = React.createClass({
    render: function() {
        return (
            <div>View</div>
        )
    }
})

var App = React.createClass({
    render: function() {
        return (
            <html>
                <head>
                    <link rel="stylesheet" href="/public/css/style.css" />
                </head>

                <Pages path={this.props.path}>
                    <Page path="/dashboard" handler={Dashboard} />
                </Pages>
            </html>
        )
    }
})

Gives me this warning:

Warning: Something is calling a React component directly. Use a factory or JSX instead. See: http://fb.me/react-legacyfactory

If I remove the <Pages /> section it works fine. Is this something to do with the way Dashboard is passed into the <Page /> component? React warnings are super helpful what with all their line numbers and stack traces and things...

The page does render however.

Looks like there are a few more things to polish but for me at least RRC with React 0.12 is working fine, just with some warnings.

@jamwaffles
Copy link
Author

I've fixed a few more warnings. There's only one left:

Warning: Something is calling a React component directly. Use a factory or JSX instead. See: http://fb.me/react-legacyfactory

This warning appears just once on startup on the server. Triggering routes and stuff doesn't show this warning. I think it's something in RRC itself.

@jocelyn-stericker
Copy link

@jamwaffles, the handler prop expects a factory. Can you see if wrapping your handlers in React.createFactory() gets rid of your warning?

For example, try <Page path="/dashboard" handler={React.createFactory(Dashboard)} />.

@jamwaffles
Copy link
Author

Sweet, that did the trick - no more errors! Would it make any sense to have the createFactory call within the <Page /> or <Location /> component itself? It would make the API cleaner. My routes currently look like this:

<Pages {...this.props}>
    <Page {...this.props} path="/dashboard" handler={React.createFactory(Dashboard)} />
    <Page {...this.props} path="/auth" handler={React.createFactory(Auth)} />

    <Page {...this.props} path="/user" handler={React.createFactory(User)} />
    <Page {...this.props} path="/user/pools" handler={React.createFactory(Pools)} />

    <NotFound {...this.props} handler={React.createFactory(Home)} />
</Pages>

I feel like I'm doing something wrong with all those {...props} for passing a bunch of stuff to views.

@STRML
Copy link
Owner

STRML commented Nov 19, 2014

Yeah, I think it should automatically create an element. As for the props,
I have been thinking about defining a field on the router that allows you
to pass props that go to all handlers. Would clean that up significantly.
On Nov 19, 2014 9:27 AM, "James Waples" [email protected] wrote:

Sweet, that did the trick - no more errors! Would it make any sense to
have the createFactory call within the or component
itself? It would make the API cleaner. My routes currently look like this:

<Pages {...this.props}>
<Page {...this.props} path="/dashboard" handler={React.createFactory(Dashboard)} />
<Page {...this.props} path="/auth" handler={React.createFactory(Auth)} />

<Page {...this.props} path="/user" handler={React.createFactory(User)} />
<Page {...this.props} path="/user/pools" handler={React.createFactory(Pools)} />

<NotFound {...this.props} handler={React.createFactory(Home)} />

I feel like I'm doing something wrong with all those {...props} for
passing a bunch of stuff to views.


Reply to this email directly or view it on GitHub
#97 (comment)
.

@jamwaffles
Copy link
Author

I wouldn't mind doing this:

<Pages {...this.props}>
    <Page path="/dashboard" handler={Dashboard} />
    <Page path="/auth" handler={Auth} />

    <Page path="/user" handler={User} />
    <Page path="/user/pools" handler={Pools} />

    <NotFound handler={Home} />
</Pages>

This assumes React.createFactory() is inside the <Page /> component.

From an implementation point of view, passing {...props} from <Pages /> should be quite easy because the handler is a React factory, right?

@STRML
Copy link
Owner

STRML commented Nov 19, 2014

Yeah, it should work. It definitely gets complicated in a way - we need to decide from an API standpoint if we require users to call React.createElement() or not. I think we are going to drop support for functions that conditionally return different elements, as well. If you need that functionality, wrap it in an element.

Sorry this has gotten kind of shunted on the wayside. I’m neck deep in a product launch and Slush in Helsinki. I will get to this as soon as I can.

On Nov 19, 2014, at 12:30 PM, James Waples [email protected] wrote:

I wouldn't mind doing this:

<Pages {...this.props}>

<Page path="/user" handler={User} />
<Page path="/user/pools" handler={Pools} />

<NotFound handler={Home} />
This assumes React.createFactory() is inside the component.

From an implementation point of view, passing {...props} from should be quite easy because the handler is a React factory, right?


Reply to this email directly or view it on GitHub #97 (comment).

@jocelyn-stericker
Copy link

I'd advocate for having react-router-component call React.createElement on the handlers.

Calling createElement for the user would also allow you to pass props down to the routes/pages without modifying the props on a ReactElement which is discouraged and they seem to be removing things that they discourage. It also might be more efficient for you to call createElement on one handler as opposed to the user calling it on all of them.

FWIW, rackt/react-router is going to be calling createElement for the user.

Good luck on your launch and thank you for this great project!

@STRML
Copy link
Owner

STRML commented Dec 19, 2014

FYI the React-0.12 branch was just updated. There's a bit of a weird issue with refs so I haven't merged into the mainline branch yet, but it should be more than usable and without warnings.

STRML added a commit that referenced this issue Dec 21, 2014
@STRML
Copy link
Owner

STRML commented Dec 27, 2014

0.23.0 and up are now React 0.12 compatible. Thanks!

@STRML STRML closed this as completed Dec 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants