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

convert React.PropTypes to "prop-types" module #628

Merged
merged 5 commits into from Apr 13, 2017

Conversation

Projects
None yet
10 participants
@dsifford
Copy link
Contributor

dsifford commented Apr 13, 2017

Closes #623

TODO:

  • (N/A) If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • (N/A) Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change
  • (N/A) If this was a change that affects the external API, update the docs and post a link to the PR in the discussion

Note: There's a significant amount of existing lint errors in the test files, but I skipped over them since that is outside the scope of this PR. None are build-breaking because those files aren't actually linted during CI.

@meteor-bot

This comment has been minimized.

Copy link

meteor-bot commented Apr 13, 2017

@dsifford: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

dsifford added some commits Apr 13, 2017

@stubailo

This comment has been minimized.

Copy link
Member

stubailo commented Apr 13, 2017

This looks great to me.

@stubailo stubailo merged commit 0754776 into apollographql:master Apr 13, 2017

3 checks passed

CLA Author has signed the Meteor CLA.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 92.386%
Details
@stubailo

This comment has been minimized.

Copy link
Member

stubailo commented Apr 13, 2017

@dsifford thanks for contributing!

@happylinks

This comment has been minimized.

Copy link

happylinks commented Apr 18, 2017

Is this released correctly? 1.0.2 doesn't seem to have these fixes.

@dacz

This comment has been minimized.

Copy link

dacz commented Apr 18, 2017

yes, 1.0.2 still has warnings, unfortunatelly

@ryancole

This comment has been minimized.

Copy link

ryancole commented Apr 19, 2017

In my bundle, the code here appears to still be pulling prop types from react.

static propTypes = {
store: PropTypes.shape({
subscribe: PropTypes.func.isRequired,
dispatch: PropTypes.func.isRequired,
getState: PropTypes.func.isRequired,
}),
client: PropTypes.object.isRequired,
immutable: PropTypes.bool,
children: PropTypes.element.isRequired,
};

@vladshcherbin

This comment has been minimized.

Copy link

vladshcherbin commented Apr 19, 2017

It was merged after 1.0.2, using master branch is an option to prevent annoying warnings.

@caffo

This comment has been minimized.

Copy link

caffo commented Apr 20, 2017

Any chance of getting a new version released? I want to upgrade react and the warnings are blocking it. 😄

@helfer

This comment has been minimized.

Copy link
Member

helfer commented Apr 21, 2017

There's a warning in one of the tests (even though they all pass) that I want to look into. I'll try to release a new version before the weekend.

@dsifford

This comment has been minimized.

Copy link
Contributor Author

dsifford commented Apr 21, 2017

Sorry, everybody! Figured converting the deprecations in just react-apollo would cause the warnings to disappear, but clearly, that was not the case.

@helfer Let me know if there's something I can do to help. I feel like this PR is not in good form since its sole purpose has not actually been achieved.

@MichaelDeBoey

This comment has been minimized.

Copy link

MichaelDeBoey commented Apr 24, 2017

@dsifford If you have refactored every React.PropTypes call to the new PropTypes call, that's all you can do in this repo.
But it's possible you still get the warnings because of dependencies haven't updated their codebase yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment