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

Update React from 0.13.1 to 0.14.7 #7083

Merged
merged 11 commits into from Mar 3, 2016
Merged

Update React from 0.13.1 to 0.14.7 #7083

merged 11 commits into from Mar 3, 2016

Conversation

@islemaster
Copy link
Member

islemaster commented Mar 2, 2016

As part of the React Wrapper Refactor work, I'm updating React from 0.13.1 to 0.14.7 (the latest version); primarily because I'd like to use react-redux which requires React 0.14+.

Facebook provides an excellent upgrade guide for v0.14 that was very helpful in explaining the warnings I got after upgrading. Fortunately, React is careful to deprecate (never break outright) between versions, so everything more-or-less "just worked" when I updated the package version and most of this change is fixing things up to remove deprecation warnings. The exception is tests, which fail if React generates any warnings so they had to be fixed up to pass.

Changes that affected us the most:

Exciting new features!

Testing

  • Manual tests
    • Verified that npm run build generates debug-mode React, and npm run build:dist generates release-mode React, in /code-studio
    • Verify that small footer renders as expected, without warnings.
    • App Lab loads/renders without warnings.
    • Verify that version history renders as expected, without warnings.
    • Verify that sendToPhone renders as expected, without warnings.
    • Verify that asset uploader renders as expected, without warnings.
    • Game Lab loads/renders without warnings
    • Verify share warnings dialog
    • Verify authored hints
    • Verify the report abuse form
    • Verify bounce
    • Verify calc
    • Verify craft
    • Verify eval
    • Verify flappy
    • Verify jigsaw
    • Verify maze
    • Verify netsim
    • Verify studio
    • Verify turtle
    • Verify contract match level
    • Verify the Bee cell editor
  • Unit tests
    • npm test passes in /apps
    • npm test passes in /code-studio
  • Integration tests
    • UI tests pass vs SauceLabs on all browsers (except i18n.feature)
      • ChromeLatestWin7
      • Chrome44XP
      • SafariYosemite
        • Except pixelation.feature (known issue)
      • IE10Win7
      • IE11Win10
      • Firefox38Yosemite
        • Except netsim_lobby.feature (passes on manual verification)
      • Firefox39Win7
        • Except netsim_lobby.feature (passes on manual verification)
      • iPhone
      • iPad
      • IE9Win7 Not testing locally.
    • Eyes tests pass vs SauceLabs on all browsers Going to let DTT handle this
"react-tools": "0.13.3",
"react": "^0.14",
"react-addons-test-utils": "^0.14",
"react-dom": "^0.14",

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 2, 2016

Author Member

Question: Should I pin to 0.14.7? Probably yes. Note that the React included here is only used in apps tests - we ship the one in code-studio/package.json.

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 2, 2016

Contributor

Pinning sounds good

@@ -74,7 +74,8 @@
"linklocal": "^2.5.2",
"lodash": "^3.10.1",
"marked": "0.3.2",
"react": "^0.13.1",
"react": "^0.14",
"react-dom": "^0.14",

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 2, 2016

Author Member

Again, probably pin this to 0.14.7? Also, isn't it scary that we were using 0.13.3 in our tests but running 0.13.1 through dashboard?

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 2, 2016

Contributor

A little bit :-P

@@ -28,7 +28,7 @@ var ColorPickerPropertyRow = React.createClass({
* Make our button a colpick color picker, if it isn't already
*/
ensureColorPicker: function () {
var element = React.findDOMNode(this.refs.colorPicker);
var element = ReactDOM.findDOMNode(this.refs.colorPicker);

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 2, 2016

Contributor

Note: I think we can get away with removing ReactDOM.findDOMNode completely in instances like this now, i.e. this.refs.colorPicker already gives us a DOM node in v0.14. Not sure if it's worth changing all of these instances.

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 2, 2016

Author Member

I see 12 instances of findDOMNode(this.refs. That's easy enough to fix and it reads much better without the extra call.

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 2, 2016

Author Member

Ah! My change of all 12 uses of findDOMNode(this.refs broke tests, and I figured out why: Refs to custom (user-defined) components work exactly as before; only the built-in DOM components are affected by this change.

So this particular case, where refs.colorPicker is a <button> we can replace this, but in some cases (like refs.age in ShareWarnings.jsx) we still have to use findDOMNode because the ref is on a user-defined component.

Fixing...

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 2, 2016

Contributor

Ahhh, TIL. I guess that maybe makes sense though. Good to know.

On Wed, Mar 2, 2016 at 11:16 AM, Brad Buchanan notifications@github.com
wrote:

In apps/src/applab/designElements/ColorPickerPropertyRow.jsx
#7083 (comment)
:

@@ -28,7 +28,7 @@ var ColorPickerPropertyRow = React.createClass({
* Make our button a colpick color picker, if it isn't already
*/
ensureColorPicker: function () {

  • var element = React.findDOMNode(this.refs.colorPicker);
  • var element = ReactDOM.findDOMNode(this.refs.colorPicker);

Ah! My change of all 12 instance broke tests, and I figured out why: Refs
to custom (user-defined) components work exactly as before; only the
built-in DOM components are affected by this change.
https://facebook.github.io/react/blog/2015/10/07/react-v0.14.html#dom-node-refs

So this particular case, where refs.colorPicker is a we can
replace this, but in some cases (like refs.age in ShareWarnings.jsx) we
still have to use findDOMNode because the ref is on a user-defined
component.

Fixing...


Reply to this email directly or view it on GitHub
https://github.com/code-dot-org/code-dot-org/pull/7083/files#r54774709.

@Bjvanminnen

This comment has been minimized.

Copy link
Contributor

Bjvanminnen commented Mar 2, 2016

looks good :) thanks for doing this!

islemaster added 5 commits Mar 2, 2016
Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of `exports`. See https://fb.me/react-warning-keys for more information.

Fixed by passing the version ID as the key to help React keep track of the rows.
Also updated component delcaration pattern to follow our convention, for easier reading in the React debug tools.
Warning: Failed form propType: You provided a `value` prop to a form field without an `onChange` handler. This will render a read-only field. If the field should be mutable use `defaultValue`. Otherwise, set either `onChange` or `readOnly`. Check the render method of `AgeDropdown`.

I solved by changing the select 'value' to 'defaultValue' which seems in the spirit of this component.  Possible future work: Why does report_abuse_form.jsx have its own AgeDropdown instead of using AgeDropdown.jsx?
@Bjvanminnen

This comment has been minimized.

Copy link
Contributor

Bjvanminnen commented on 314e0ff Mar 2, 2016

lgtm. Did you test that this works properly?

I think the answer to your question is that AgeDropdown.jsx is in apps and report_abuse_form is not.

This comment has been minimized.

Copy link
Member Author

islemaster replied Mar 2, 2016

I tested that this sets the initial value of that select field to {age}. I wasn't sure what the eventual desired effect of this age property in the report abuse form was.

This comment has been minimized.

Copy link
Contributor

Bjvanminnen replied Mar 2, 2016

Just a simple dropdown that lets you select your age. I suspect the reason we never had problems with values vs. defaultValue is that we never ended up re-rendering report_abuse_form

islemaster added 4 commits Mar 2, 2016
Stop using [renderToStaticMarkup](https://facebook.github.io/react/docs/top-level-api.html#reactdomserver.rendertostaticmarkup) to render course progress because it's not supposed to be used in client code, and has been [moved to ReactDOMServer in React 0.14](https://facebook.github.io/react/blog/2015/10/07/react-v0.14.html#two-packages-react-and-react-dom).

Instead, create and prepend a mount point and render into that using regular ReactDOM.render.

This may affect an [old TODO from 6343](#6343 (comment)).  FYI @joshlory.
Again replacing with creating a mountPoint and using ReactDOM.render() to render inside it.

FYI @joshlory
Any time React renders an array of components, each component in the array must have a "key" property that helps React manage the components efficiently.  I'm doing a sort of clumsy update here to meet this requirement - there may be a cleaner way to restructure these.

Also updated the Authored Hints components to follow our new declaration standard, so their names show up nicely in the React dev tools.

FYI @Hamms
Warning: Failed form propType: You provided a `value` prop to a form field without an `onChange` handler. This will render a read-only field. If the field should be mutable use `defaultValue`. Otherwise, set either `onChange` or `readOnly`. Check the render method of `TypeChooser`.
Fix: Changed select "value" to "defaultValue".

Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of `exports`. See https://fb.me/react-warning-keys for more information.
Fix: Add "key" property to <div> returned by map function.

FYI @bcjordan
@bcjordan

This comment has been minimized.

Copy link
Contributor

bcjordan commented on 08d0d8e Mar 2, 2016

LGTM, thanks!

selectedX and selectedY are no longer required props on the Grid component (they are not provided in the initial state, so it doesn't make sense for them to be required).

Provide unique key prop to components returned by map functions (which helps React with tracking, internally).

FYI @Hamms
@Hamms

This comment has been minimized.

Copy link
Contributor

Hamms commented on 2f5d672 Mar 3, 2016

Much appreciated!

@Hamms

This comment has been minimized.

Copy link
Contributor

Hamms commented on 19f99b0 Mar 3, 2016

Does React have an idiomatic way to handle the keys when the array merely represents adjacent components rather than groups of similar components?

This comment has been minimized.

Copy link
Member Author

islemaster replied Mar 3, 2016

That's a good question. I'll look into it.

This comment has been minimized.

Copy link
Member Author

islemaster replied Mar 3, 2016

I'm having trouble finding any example of using an array for adjacent, nonsimilar components; there seems to be some assumption in the library that an array of components will by dynamic and similar, hence the new warning suggesting that you use keys.

There are some idiomatic patterns for conditionally including a component, which is what you're really trying to do here. The official docs suggest ternary expressions, temporary variables assigned to single components, or inline IIFEs.

This old issue suggests a fourth option that I particularly like: Shortcut conditionals are apparently a dependable way to conditionally include components. So you might rewrite the render method something like this:

var body;
if (this.props.renderedMarkdown) {
  // This path is fine...
} else {
  body = <div>
    <p className='dialog-title'>{ this.props.puzzleTitle }</p>
    {!!this.props.instructions && <p dangerouslySetInnerHTML={{ __html: this.props.instructions }}/>}
    {!!this.props.instructions2 && <p className='instructions2' dangerouslySetInnerHTML={{ __html: this.props.instructions2 }}/>}
  </div>;
}
// yada yada yada...
return <div>
  {body}
  {aniGif}
  {this.props.authoredHints}
</div>

Also, I know that introduces an extra <div> but as long as it doesn't disrupt layout I find extra wrapper layers are often my go-to in React.

This comment has been minimized.

Copy link
Contributor

Hamms replied Mar 3, 2016

Yeah, the minor ugliness of yet another nesting div is much prettier than the array hackiness I'm doing.

I'm definitely doing that hackiness in a couple places other than here; I'll go through and update them to use either temporary variables or shortcut conditions.

Thank you very much for doing this research!

@islemaster

This comment has been minimized.

Copy link
Member Author

islemaster commented Mar 3, 2016

I'm declaring this thing tested and merging it. Look out below!

islemaster added a commit that referenced this pull request Mar 3, 2016
Update React from 0.13.1 to 0.14.7
@islemaster islemaster merged commit 64c04ab into staging Mar 3, 2016
5 checks passed
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 increased (+0.0%) to 85.114%
Details
hound No violations found. Woof!
@islemaster islemaster deleted the react-14 branch Mar 3, 2016
deploy-code-org added a commit that referenced this pull request Mar 3, 2016
64c04ab Merge pull request #7083 from code-dot-org/react-14 (Brad Buchanan)
e1d5d1a Automatically built. (Continuous Integration)
b049329 Merge pull request #7121 from code-dot-org/setKeyTooLarge2 (Brent Van Minnen)
5d2466d Merge pull request #6978 from code-dot-org/rubocopVoid (ashercodeorg)
dea603f address code review feedback and add a test (Brent Van Minnen)
faa1532 Update schema cache dump after schema changes. (Continuous Integration)
660c8e5 Merge pull request #7072 from code-dot-org/add-level-notes-to-levelbuilder (Elijah Hamovitz)
9aaa141 Better error for setKeyValue exceeding data limits (Brent Van Minnen)
51c178b Merge pull request #7118 from code-dot-org/hourofcode_events_memory_fix (Will Jordan)
9c34f57 Merge pull request #7119 from code-dot-org/levelbuilder (Will Jordan)
236889b levelbuilder content changes (-will) (Continuous Integration)
7c19fef staging content changes (-will) (Continuous Integration)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.