If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

upgrade React to 0.13.3

RESOLVED FIXED in Firefox 45

Status

Hello (Loop)
Client
P2
normal
Rank:
25
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: dmose, Assigned: dmose)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: [tech-debt, timebox])

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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).
(Assignee)

Comment 1

3 years ago
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.
backlog: --- → backlog+
Rank: 25
OS: Mac OS X → All
Priority: -- → P2
Hardware: x86 → All
(Assignee)

Comment 2

2 years ago
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.
Summary: upgrade React to 0.13.1 → upgrade React to 0.13.3

Updated

2 years ago
Whiteboard: [tech-debt, timebox]
(Assignee)

Updated

2 years ago
Depends on: 1220878
(Assignee)

Comment 3

2 years ago
Created attachment 8682820 [details] [diff] [review]
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.
Attachment #8682820 - Flags: review?(standard8)
(Assignee)

Comment 4

2 years ago
(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.
(Assignee)

Updated

2 years ago
Assignee: nobody → dmose
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.
Attachment #8682820 - Flags: review?(standard8) → review+
(Assignee)

Comment 6

2 years ago
(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"
(Assignee)

Comment 7

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

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/646659e24772
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.