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

Upgrade react-native-compat to avoid PropTypes warnings #1673

Merged
merged 3 commits into from
Aug 17, 2017

Conversation

atticoos
Copy link
Member

@atticoos atticoos commented Aug 17, 2017

One step towards #1540 by updating the react-native-compat dependency to move off of React.PropTypes (atticoos/react-native-compat#1). There still needs to be a greater sweep to remove usages of React.PropTypes -- users will likely still see this warning on their devices.

What I did

Upgrades to react-native-compat@v0.0.4, which uses prop-types.

How to test

I manually pulled down react-native-compat and compared the old vs new version to confirm the warning goes away. However, as mentioned, that may not entirely fix the warnings throughout Storybook we still have occurrences using it).

@atticoos atticoos changed the title upgrade react-native-compat to avoid PropTypes warnings Upgrade react-native-compat to avoid PropTypes warnings Aug 17, 2017
@@ -56,7 +56,7 @@
"json-loader": "^0.5.4",
"json5": "^0.5.1",
"postcss-loader": "^2.0.5",
"react-native-compat": "0.0.2",
"react-native-compat": "0.0.4",
Copy link
Member

Choose a reason for hiding this comment

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

do you plan on versioning according to semvar? we could do ~0.0.4 so we don't have to do a PR for every minor fix.

Copy link
Member Author

@atticoos atticoos Aug 17, 2017

Choose a reason for hiding this comment

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

Yes - this is currently in a prerelease stage so we'd need to upgrade to 1.x for precedence options (unless that has since changed)

Let me update react-native-compat to 1.0.0 so we can take advantage of ~

Copy link
Member Author

Choose a reason for hiding this comment

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

Any strong opinions on tilde vs caret? I generally go with a tilde, but given the pattern of this package file, I added a caret.

Copy link
Member

Choose a reason for hiding this comment

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

Think it really depends on how you plan to maintain it. Automatic updates should not be breaking changes. Typically ^ minor version updates are feature improvements, ~ are bug fixes. If you follow the same convention, ^ should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

The project is semver compliant 👍

@codecov
Copy link

codecov bot commented Aug 17, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@628ee9d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1673   +/-   ##
=========================================
  Coverage          ?   21.13%           
=========================================
  Files             ?      247           
  Lines             ?     5582           
  Branches          ?      671           
=========================================
  Hits              ?     1180           
  Misses            ?     3917           
  Partials          ?      485

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 628ee9d...40e7f97. Read the comment docs.

@Hypnosphi Hypnosphi merged commit c67ebdd into master Aug 17, 2017
@Hypnosphi Hypnosphi deleted the latest-react-native-compat branch August 17, 2017 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants