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

[react] Use React 15.5 and prop-types lib #6577

Merged
merged 10 commits into from
Apr 13, 2017
Merged

[react] Use React 15.5 and prop-types lib #6577

merged 10 commits into from
Apr 13, 2017

Conversation

rsolomon
Copy link
Contributor

@rsolomon rsolomon commented Apr 11, 2017

Use new prop-types lib instead of the deprecated React.PropTypes.

Fixes #6542

Note that the react-title-component lib references in docs still uses React.PropTypes, so you'll still get the error there.

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

@muibot muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Apr 11, 2017
@rsolomon
Copy link
Contributor Author

I think this might be blocked until Enzyme releases an update. Relevant PR is here, but it hasn't yet been released.

@mbrookes
Copy link
Member

Thanks for taking this on. 👍

@oliviertassinari
Copy link
Member

As any responsible developers, we treat warning as error in our test suite thanks to test/utils/consoleError.js. I have been commenting out that logic on the next branch.
I'm not against doing so on the master branch, waiting for the warning to be fixed in our dependencies.

@rsolomon
Copy link
Contributor Author

@oliviertassinari I commented out the relevant bits and slapped a comment in there. Let me know if there's anything else I can do to prep this PR.

Note also that we'll still be seeing deprecation warnings in docs until this or this PR merges for react-document-title. It's also an option to switch to the more actively maintained react-document-title, but it's also waiting on a pull request to bring it up to date.

@muibot muibot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: Needs Review labels Apr 11, 2017
@muibot muibot added PR: Needs Review and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Apr 11, 2017
@mbrookes
Copy link
Member

@rsolomon Sorry about the merge conflict. Thanks for the quick turnround!

@mbrookes mbrookes removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Apr 11, 2017
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rsolomon Thanks for updating the PR. Some last comments.

@@ -1,4 +1,6 @@
import React, {Component, PropTypes} from 'react-native';
import React, {Component} from 'react-native';

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor issue, we have quite some blank lines added by this PR between the imports.

package.json Outdated
"react-dom": "^15.4.0",
"react": "^15.5.0",
"react-dom": "^15.5.0",
"react-prop-types": "^15.5.0",
Copy link
Member

@oliviertassinari oliviertassinari Apr 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it should be a dependency. No need to worry about conflicts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For instance: reduxjs/react-redux#663.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It should be a dependency.
  2. The package is called prop-types, not react-prop-types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😰 Yikes, thanks for catching.

package.json Outdated
"react-dom": "^15.4.0",
"react": "^15.5.0",
"react-dom": "^15.5.0",
"react-prop-types": "^15.5.0",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. It should be a dependency.
  2. The package is called prop-types, not react-prop-types.

package.json Outdated
@@ -52,6 +51,7 @@
"keycode": "^2.1.8",
"lodash.merge": "^4.6.0",
"lodash.throttle": "^4.1.1",
"prop-types": "^15.5.0",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use ^15.5.7. There are critical known issues in earlier versions.

@rsolomon
Copy link
Contributor Author

@oliviertassinari the whitespace and dependency issues should be good to go. Let me know if there's anything I can do to help

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks almost good to me 👍 .

Also noticed that you left the docs/package.json untouched. We can do that in another PR.

package.json Outdated
"react": "^15.4.0",
"react-dom": "^15.4.0",
"react": "^15.5.0",
"react-dom": "^15.5.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we stick to the older versions? I don't think that we need to upgrade the requirement. We would have to release a minor instead of a patch otherwise.

    "react": "^15.4.0",
    "react-dom": "^15.4.0",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah you're right for the peerDependencies. I do think you'll want to keep the dependencies (though I think in this case they should be devDependencies, but I don't have any historical context here) to 15.5.0 for react-dom because this diff uses react-dom/test-utils, which was a recent migration.

Just want to confirm with you before I make the change.

Copy link
Member

@oliviertassinari oliviertassinari Apr 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are referring to the following dependencies:

+    "react-addons-create-fragment": "^15.5.0",
+    "react-addons-transition-group": "^15.5.0",

Same, I don't think that we need to update those. If we do, let's release a minor.

@muibot muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Apr 13, 2017
@rsolomon
Copy link
Contributor Author

rsolomon commented Apr 13, 2017

@oliviertassinari Some quick notes on the last update:

  • I've reverted the peerDependencies back to 15.4.0, as no changes to the non-test code were made that necessitate an update.
  • For tests to pass without warnings, we have to pull TestUtils from the react-dom package, which was added in 15.5.0. This version of react-dom requires its peer React be of version ^15.5.0 as well, so that was bumped in devDependencies as well.
  • I was able to upgrade Enzyme to get rid of the need to comment out the warning -> error module, but it depends on react-dom@^15.5.0, so it will play a role in deciding what to do.

I'm not completely comfortable setting the devDependencies higher than the actual deployable module dependencies, but I'm not sure if that's a dealbreaker or not. If it is, we have a few options:

  1. Keep everything in both dependencies and devDependencies to ^15.4.0. This will remove warnings from the module itself. Unfortunately the tests will need to depend on the deprecated react-addons-test-utils. That has the implication of having both the material-ui tests and the Enzyme dependency throw deprecation warnings, requiring us to keep the throw on warning code commented out.

  2. Update all dependencies to ^15.5.0, requiring a minor version bump (hey, 18 is a lucky number in some cultures I'm sure!)

  3. Keep the dependency versions higher for development than for actual deployment. This has some downsides I'm sure, but it might not be too bad considering all of the effort is being put into next anyway.

@mbrookes mbrookes merged commit 912649d into mui:master Apr 13, 2017
@oliviertassinari oliviertassinari added PR: Review Accepted and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Apr 13, 2017
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 13, 2017

@rsolomon Option 3 sounds fine to me. The more version of React we support, the better. We can always change the strategy if that introduce issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PropTypes via the main React package is deprecated
6 participants