1147167 - upgrade React to 0.13.3

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Dan Mosedale (:dmosedale, :dmose)]

http://facebook.github.io/react/blog/2015/03/16/react-v0.13.1.html has links to the latest bits.  The meat of the changes are described at http://facebook.github.io/react/blog/2015/03/10/react-v0.13.html because they actually happened in the previous version.

It doesn't look to me like there's anything that will obviously trip us up while upgrading. 

Assuming that we are going to continue to care about IE 8 for the "not supported, go get Firefox" case,  will want to consider using --target es3, or perhaps simply creating a static page for that.

Given the unusual way that ContactsList handles state, it may be worth a few extra minutes to ensure that the various set state related changes  don't cause any problems here.

We'll probably want to file some spinoffs bugs about deprecated stuff, in particular:

* ComponentClass.type -> Component.Class
* alternatives to React.addons.classSet (one is suggested in the rel notes, and it could be that prouget, jlongster, or someone at MoFo will have a recommendation here).

and we'll also probably want to spin off a bug to evaluate what we want to do on the ES 6 front.  http://facebook.github.io/react/blog/2015/01/27/react-v0.13.0-beta-1.html points out that react-tools actually ships with a transpiler that we could use, and there are various implications of doing that.  This would involve giving up mixins, so I expect we'll want to wait for things to settle out around that (see <https://github.com/facebook/react/issues/1380#issuecomment-85346113> for some discussion).

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Giving this a Firefox backlog+, as React is moving quickly enough that we don't want to get too far behind, or we'll be hurting when future versions start coming out and killing some of the currently-deprecated features.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

We actually want 0.13.3, as it's the latest stable.  (0.14 may be out by the time we pick up this bug, but I suspect we want to make the upgrades happen in separate pieces anyway, to bound both the code changes required and the amount of risk that comes with any upgrade.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: Switch Hello from React 0.12.2 to 0.13.3

This is built on top of bug 1220878, and needs to be applied on top of that.
The patch is a simple upgrade, with a few unit test fixes to unbreak them
and make them less fragile.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #0)
> http://facebook.github.io/react/blog/2015/03/16/react-v0.13.1.html has links
> to the latest bits.  The meat of the changes are described at
> http://facebook.github.io/react/blog/2015/03/10/react-v0.13.html because
> they actually happened in the previous version.
>
> It doesn't look to me like there's anything that will obviously trip us up
> while upgrading. 
> 
> Assuming that we are going to continue to care about IE 8 for the "not
> supported, go get Firefox" case,  will want to consider using --target es3,
> or perhaps simply creating a static page for that.

I've used --target=es3, and it seems to be working fine.
 
> Given the unusual way that ContactsList handles state, it may be worth a few
> extra minutes to ensure that the various set state related changes  don't
> cause any problems here.

This code is no longer relevant.

> We'll probably want to file some spinoffs bugs about deprecated stuff, in
> particular:
> 
> * ComponentClass.type -> Component.Class

We don't actually use this.

> * alternatives to React.addons.classSet (one is suggested in the rel notes,
> and it could be that prouget, jlongster, or someone at MoFo will have a
> recommendation here).

Already handled in another bug.

> and we'll also probably want to spin off a bug to evaluate what we want to
> do on the ES 6 front. 
> http://facebook.github.io/react/blog/2015/01/27/react-v0.13.0-beta-1.html
> points out that react-tools actually ships with a transpiler that we could
> use, and there are various implications of doing that.  This would involve
> giving up mixins, so I expect we'll want to wait for things to settle out
> around that (see
> <https://github.com/facebook/react/issues/1380#issuecomment-85346113> for
> some discussion).

I don't actually think it's important to spin this bug off now.  We've got plenty of other stuff on our minds, and we're unlikely to lose sight of it given that an increasing number of examples and such on the web are likely to be using ES6 classes.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8682820 [details] [diff] [review]
Switch Hello from React 0.12.2 to 0.13.3

Review of attachment 8682820 [details] [diff] [review]:
-----------------------------------------------------------------

r=Standard8 once the comment I just added to bug 1220878 is fixed. Also a couple of minor comments below to consider.

::: browser/components/loop/content/js/panel.jsx
@@ +619,5 @@
>    });
>  
>    /**
>     * User profile prop can be either an object or null as per mozLoopAPI
> +   * and there is no way to express this with React 0.13.3

Doesn't this really boil down to "React.PropTypes.object" with no .isRequired?

I realise it isn't really part of this bug, but the comment seems strange, and we could just fix it if that's the case.

::: browser/components/loop/content/shared/libs/react-0.13.3-prod.js
@@ +1,2 @@
> +/**
> + * React (with addons) v0.13.3

You seem to have git mv the non-prod version, but not the prod version. I think it'd be clear to git mv both, even if the diff is horrible.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Mark Banner (:standard8) from comment #5)
> Comment on attachment 8682820 [details] [diff] [review]
> Switch Hello from React 0.12.2 to 0.13.3
> 
> Review of attachment 8682820 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=Standard8 once the comment I just added to bug 1220878 is fixed. Also a
> couple of minor comments below to consider.
> 
> ::: browser/components/loop/content/js/panel.jsx
> @@ +619,5 @@
> >    });
> >  
> >    /**
> >     * User profile prop can be either an object or null as per mozLoopAPI
> > +   * and there is no way to express this with React 0.13.3
> 
> Doesn't this really boil down to "React.PropTypes.object" with no
> .isRequired?

It depends how precise one wants to be, since null is different than value of undefined is different than no value passed, though all are falsy.  

> I realise it isn't really part of this bug, but the comment seems strange,
> and we could just fix it if that's the case.

Rather than spend time trying to nail down whether that level of precision matters or not (I suspect it doesn't, but...), I figured I'd just leave it alone, which still seems like a reasonable plan to me.  :-)

> ::: browser/components/loop/content/shared/libs/react-0.13.3-prod.js
> @@ +1,2 @@
> > +/**
> > + * React (with addons) v0.13.3
> 
> You seem to have git mv the non-prod version, but not the prod version. I
> think it'd be clear to git mv both, even if the diff is horrible.

So that was an accident, and I'm actually not sure how I did it.  I just spent a bit of time trying make the -prof file be moved as well (without success), but the more I think about it, the more that feels wrong: we're not just moving a reference to an existing file, the contents are changing too.  That said, I'm having a very hard time imagining that spending any more effort on this is likely to pay off in the future: it's hard to read either history as anything other than "we upgraded from 0.12.2 to 0.13.3"

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

https://hg.mozilla.org/integration/fx-team/rev/646659e2477278ef83bf78fa2c45855c61198e7e
Bug 1147167-Upgrade React in Hello to 0.13.3, r=Standard8

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/646659e24772