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

Upgrade to React 15.0.2 #8178

Merged
merged 7 commits into from May 6, 2016

Conversation

Projects
None yet
6 participants
@islemaster
Member

islemaster commented May 3, 2016

Updates React, React DOM, and React Test Utils from 0.14.7 to 15.0.2 (not as big as it sounds; 0.14.7 to 15.0.0 is a single version bump).

Major benefits of upgrading include performance improvements and full SVG support. It's also least painful now while we're still fairly current.

The one dependency blocking this update was react-color, which has an open pull request to support React 15 but the maintainer has been unavailable for some time. I forked react-color and fixed up the v15 support, a change which I'll push upstream once we feel it's thoroughly tested.

Testing

  • apps and code-studio tests are passing.
  • UI tests (sans eyes) are passing on ChromeLatestWin7, Firefox45Win7, SafariYosemite, IE11Win10
  • Manual verification of App Lab and gamelab revealed one problem with the updated color picker (fixed now), otherwise everything looks great and warning-free.
  • Manually tested authored hints workflow
  • Manually tested Karel grid editor
  • I could use help finding the other stuff that needs coverage.

To-do

  • Pass netsim_lobby.feature on Firefox45Win7
  • Pin code-dot-org/react-color dependency to a particular commit/branch
  • Measure and report change built package size
  • Understand why applab-api.js includes a copy of React when it didn't before.
@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster May 3, 2016

Member

@mehalshah @aoby @Hamms @Bjvanminnen - Please take a look, and help brainstorm other uses of React that probably need manual verification. In particular, I'm not too familiar with the new bits of dashboard that are using React.

Member

islemaster commented May 3, 2016

@mehalshah @aoby @Hamms @Bjvanminnen - Please take a look, and help brainstorm other uses of React that probably need manual verification. In particular, I'm not too familiar with the new bits of dashboard that are using React.

Show outdated Hide outdated apps/package.json
@Bjvanminnen

This comment has been minimized.

Show comment
Hide comment
@Bjvanminnen

Bjvanminnen May 3, 2016

Contributor

Worth double checking that our built packages didnt significantly change in size (i.e. that we're not somehow requiring two different versions of React now bc some third party is still trying to use an old version)

Contributor

Bjvanminnen commented May 3, 2016

Worth double checking that our built packages didnt significantly change in size (i.e. that we're not somehow requiring two different versions of React now bc some third party is still trying to use an old version)

@Bjvanminnen

This comment has been minimized.

Show comment
Hide comment
@Bjvanminnen

Bjvanminnen May 3, 2016

Contributor

modulo a couple of comments, things look good from my side. Thanks for doing this Brad!

Contributor

Bjvanminnen commented May 3, 2016

modulo a couple of comments, things look good from my side. Thanks for doing this Brad!

@mehalshah

This comment has been minimized.

Show comment
Hide comment
@mehalshah

mehalshah May 3, 2016

Contributor

None of the new PLC stuff uses react, you don't have to worry about that

Contributor

mehalshah commented May 3, 2016

None of the new PLC stuff uses react, you don't have to worry about that

@aoby

This comment has been minimized.

Show comment
Hide comment
@aoby

aoby May 3, 2016

Collaborator

lgtm. Since my stuff isn't in staging yet anyway, I'll rebase and verify that everything still works as expected.

Collaborator

aoby commented May 3, 2016

lgtm. Since my stuff isn't in staging yet anyway, I'll rebase and verify that everything still works as expected.

@Hamms

This comment has been minimized.

Show comment
Hide comment
@Hamms

Hamms May 3, 2016

Contributor

The authored hints workflow (most easily tested here https://studio.code.org/s/allthethings/stage/6/puzzle/2) and the new Karel grid editor (best tested by editing any Bee level) both use React.

Contributor

Hamms commented May 3, 2016

The authored hints workflow (most easily tested here https://studio.code.org/s/allthethings/stage/6/puzzle/2) and the new Karel grid editor (best tested by editing any Bee level) both use React.

@@ -40,7 +40,7 @@ var ColorPickerPropertyRow = React.createClass({
handleColorChange: function (color) {
if (color.rgb.a == 1) {
// no transparency set
this.changeColor('#' + color.hex);
this.changeColor(color.hex);

This comment has been minimized.

@islemaster

islemaster May 3, 2016

Member

See casesandberg/react-color#51. Although react-color is supposed to return hex values with a leading # since 1.3.8, there's a bug in the published npm 2.0.0 version where the transpiled lib is out of date and returns hex colors without the leading #. Our fork actually has the expected behavior and returns hex with leading # so we don't need to add it here anymore.

@islemaster

islemaster May 3, 2016

Member

See casesandberg/react-color#51. Although react-color is supposed to return hex values with a leading # since 1.3.8, there's a bug in the published npm 2.0.0 version where the transpiled lib is out of date and returns hex colors without the leading #. Our fork actually has the expected behavior and returns hex with leading # so we don't need to add it here anymore.

// Reacquire reference to textArea, since it was unmounted when we
// selected the screen.
textArea = $('#propertyRowContainer textarea').first()[0];

This comment has been minimized.

@islemaster

islemaster May 3, 2016

Member

This actually appears to be an existing "bug" in this test - depending on a particular element persisting across renders in a way that React doesn't guarantee, which broke in the upgrade. I worry a little that we'll see this sort of problem elsewhere, though in our production React code we rarely depend on this sort of persistence (it's not idiomatic).

@islemaster

islemaster May 3, 2016

Member

This actually appears to be an existing "bug" in this test - depending on a particular element persisting across renders in a way that React doesn't guarantee, which broke in the upgrade. I worry a little that we'll see this sort of problem elsewhere, though in our production React code we rarely depend on this sort of persistence (it's not idiomatic).

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster May 3, 2016

Member

Measured package sizes, before and after:

apps/build/package/js code-studio/build/js
staging@8a9a404 85M (86200) 2.1M (2124)
with React 15.0.1 87M (88484) 2.1M (2144)

2MB increase to the apps package javascript seems bad - especially since we shouldn't really be including React in apps release build, right?

Member

islemaster commented May 3, 2016

Measured package sizes, before and after:

apps/build/package/js code-studio/build/js
staging@8a9a404 85M (86200) 2.1M (2124)
with React 15.0.1 87M (88484) 2.1M (2144)

2MB increase to the apps package javascript seems bad - especially since we shouldn't really be including React in apps release build, right?

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster May 3, 2016

Member

Here's a more detailed diff of the size difference in the apps package JS.

< 10240 build/package/js/applab-api.js
> 11880 build/package/js/applab-api.js

< 76    build/package/js/fa_ir/studio_locale.js
> 80    build/package/js/fa_ir/studio_locale.js

< 8160  build/package/js/applab-api-cache.json
> 8800  build/package/js/applab-api-cache.json

< 7072  build/package/js/applab.js
> 7076  build/package/js/applab.js

< 72    build/package/js/es_es/studio_locale.js
> 68    build/package/js/es_es/studio_locale.js

So the bulk of the size increase occurs in applab-api.js and applab-api-cache.json. That seems okay to me since they're not files we load regularly (I was worried common.js had been affected).

@pcardune any ideas about why the React 15 update would have such a large impact on the applab-api.js file?

Member

islemaster commented May 3, 2016

Here's a more detailed diff of the size difference in the apps package JS.

< 10240 build/package/js/applab-api.js
> 11880 build/package/js/applab-api.js

< 76    build/package/js/fa_ir/studio_locale.js
> 80    build/package/js/fa_ir/studio_locale.js

< 8160  build/package/js/applab-api-cache.json
> 8800  build/package/js/applab-api-cache.json

< 7072  build/package/js/applab.js
> 7076  build/package/js/applab.js

< 72    build/package/js/es_es/studio_locale.js
> 68    build/package/js/es_es/studio_locale.js

So the bulk of the size increase occurs in applab-api.js and applab-api-cache.json. That seems okay to me since they're not files we load regularly (I was worried common.js had been affected).

@pcardune any ideas about why the React 15 update would have such a large impact on the applab-api.js file?

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster May 3, 2016

Member

Scanning the diff of applab-api.js, it looks like an unminified copy of React is being included that wasn't there before (and I don't see a minified copy being removed either).

Member

islemaster commented May 3, 2016

Scanning the diff of applab-api.js, it looks like an unminified copy of React is being included that wasn't there before (and I don't see a minified copy being removed either).

@pcardune

This comment has been minimized.

Show comment
Hide comment
@pcardune

pcardune May 3, 2016

Collaborator

That's odd. I'm not sure what could be causing that. Perhaps it has something to do with the way applab-api.js gets built. Did you run grunt clean first? That's the only thing I can think of.

Collaborator

pcardune commented May 3, 2016

That's odd. I'm not sure what could be causing that. Perhaps it has something to do with the way applab-api.js gets built. Did you run grunt clean first? That's the only thing I can think of.

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster May 3, 2016

Member

I did. Measurements were taken by running

rm -rf node_modules
npm install
npm run build:dist
du // or some variant thereof

I'll make sure I understand this before we upgrade.

Member

islemaster commented May 3, 2016

I did. Measurements were taken by running

rm -rf node_modules
npm install
npm run build:dist
du // or some variant thereof

I'll make sure I understand this before we upgrade.

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster May 5, 2016

Member

I may have a lead on the mystery of the extra 2MB in applab-api.js. Our react-color@2.0.0 fork uses React 15.0.2, but it also depends on react-css@0.4.5 which in turn depends on react@0.14.8. I think we're getting two different copies of React in the bundle. Working to resolve...

Member

islemaster commented May 5, 2016

I may have a lead on the mystery of the extra 2MB in applab-api.js. Our react-color@2.0.0 fork uses React 15.0.2, but it also depends on react-css@0.4.5 which in turn depends on react@0.14.8. I think we're getting two different copies of React in the bundle. Working to resolve...

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster May 5, 2016

Member

In fact, applab-api.js on staging right now contains both React 0.14.7 and React 0.14.8 (just look for @providesModule ReactVersion). After this change it contains 0.14.8 and 15.0.2. I suspect browserify is deduplicating modules, and there's a lot more duplication between 0.14.7 and 0.14.8 than there is between 0.14.8 and 15.0.2, which may explain the discrepancy.

Here's the easiest way to see that two version are installed:

[cdo]/apps> npm ls react
blockly-mooc@0.0.161 /home/brad/Projects/code-dot-org/apps
├── react@0.14.7 
└─┬ react-color@2.0.0
  └── react@0.14.8 

I'll take a shot at fixing up react-css to support React 15+ and see if that solves anything.

Member

islemaster commented May 5, 2016

In fact, applab-api.js on staging right now contains both React 0.14.7 and React 0.14.8 (just look for @providesModule ReactVersion). After this change it contains 0.14.8 and 15.0.2. I suspect browserify is deduplicating modules, and there's a lot more duplication between 0.14.7 and 0.14.8 than there is between 0.14.8 and 15.0.2, which may explain the discrepancy.

Here's the easiest way to see that two version are installed:

[cdo]/apps> npm ls react
blockly-mooc@0.0.161 /home/brad/Projects/code-dot-org/apps
├── react@0.14.7 
└─┬ react-color@2.0.0
  └── react@0.14.8 

I'll take a shot at fixing up react-css to support React 15+ and see if that solves anything.

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster May 5, 2016

Member

Okay, after forking and upgrading reactcss to support React 15 and confirming that we only install one copy of React now, the applab-api.js bundle actually shrunk by about a megabyte some amount. I'll make sure code-studio is good too and post some new measurements.

Member

islemaster commented May 5, 2016

Okay, after forking and upgrading reactcss to support React 15 and confirming that we only install one copy of React now, the applab-api.js bundle actually shrunk by about a megabyte some amount. I'll make sure code-studio is good too and post some new measurements.

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster May 5, 2016

Member

New measurements. Measured staging at the last commit I merged into this branch. Also corrected somewhat by excluding the *cache.json files from the apps package sizes (we should put these outside the package directory).

apps/build/package/js (sans cachefiles) applab-api.js code-studio/build/js
staging @ a6fb5c0 61504K 10348K 2124K
This PR 61420K 10264K 2148K
Delta -84K -84K +24K

These numbers are much closer to what I expect:

  • code-studio grows by 24K because React 15.0.2 is slightly larger than React 0.14.7
  • applab-api.js shrinks by 84K because it used to include two (nearly identical) versions of React and now it only includes one.
  • The decrease in the apps package is entirely accounted for by applab-api.js, because the rest of apps doesn't include React (it uses code-studio's copy).
Member

islemaster commented May 5, 2016

New measurements. Measured staging at the last commit I merged into this branch. Also corrected somewhat by excluding the *cache.json files from the apps package sizes (we should put these outside the package directory).

apps/build/package/js (sans cachefiles) applab-api.js code-studio/build/js
staging @ a6fb5c0 61504K 10348K 2124K
This PR 61420K 10264K 2148K
Delta -84K -84K +24K

These numbers are much closer to what I expect:

  • code-studio grows by 24K because React 15.0.2 is slightly larger than React 0.14.7
  • applab-api.js shrinks by 84K because it used to include two (nearly identical) versions of React and now it only includes one.
  • The decrease in the apps package is entirely accounted for by applab-api.js, because the rest of apps doesn't include React (it uses code-studio's copy).
@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster May 5, 2016

Member

@Bjvanminnen your concern about two versions of React was well-founded - it just happens that we already had that problem. This PR will fix it. Any other concerns before we upgrade?

Member

islemaster commented May 5, 2016

@Bjvanminnen your concern about two versions of React was well-founded - it just happens that we already had that problem. This PR will fix it. Any other concerns before we upgrade?

@Bjvanminnen

This comment has been minimized.

Show comment
Hide comment
@Bjvanminnen

Bjvanminnen May 5, 2016

Contributor

Nope. Thanks for doing the work to track this down!

Contributor

Bjvanminnen commented May 5, 2016

Nope. Thanks for doing the work to track this down!

@islemaster islemaster merged commit fa36f2a into staging May 6, 2016

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 84.705%
Details
hound No violations found. Woof!

@islemaster islemaster deleted the react-15.0.1 branch May 6, 2016

deploy-code-org added a commit that referenced this pull request May 6, 2016

Automatically built.
fa36f2a Merge pull request #8178 from code-dot-org/react-15.0.1 (Brad Buchanan)
a414487 Merge pull request #8241 from code-dot-org/visualizationEditor (Brent Van Minnen)
05fb772 Update schema cache dump after schema changes. (Continuous Integration)
6b828cc Merge pull request #8171 from code-dot-org/nps2015 (ashercodeorg)
393f196 PR feedback. (Asher Kach)
a0c2b36 PR feedback. (Asher Kach)
7b1b288 Merge conflict (update timestamp in schema.rb). (Asher Kach)
5b1c657 Fix typo. (Asher Kach)

islemaster added a commit that referenced this pull request Aug 24, 2016

Upgrade react-color from 2.2.2 to 2.2.5
[Changelog](https://github.com/casesandberg/react-color/blob/master/CHANGELOG.md)

There shouldn't be any user-facing change here.  The major reason I'm excited about this upgrade is that it [bumps reactcss from 0.4.3 to 1.0.6](casesandberg/react-color#201), which finally deduplicates React and Lodash within react-color and should reduce our overall bundle size, closing the book on a battle I've been waging since we updated to React 15 in #8178.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment