Skip to content
This repository has been archived by the owner on May 19, 2020. It is now read-only.

Add prop-types package and update code #1150

Merged
merged 1 commit into from
Jul 6, 2017
Merged

Conversation

el-mapache
Copy link
Contributor

  • React.PropTypes is deprecated and was throwing a warning in the
    console.

Just a lil day off coding..

There are a lot of changes but they can be boiled down to removing the React prefix from PropTypes and importing the prop-types library. I used React codemod so I feel pretty confident about not missing anything.

@el-mapache el-mapache self-assigned this Jul 5, 2017
* React.PropTypes is deprecated and was throwing a warning in the
console.
@el-mapache el-mapache force-pushed the ab-add-prop-types-package branch from 47f47b1 to 60a5531 Compare July 5, 2017 15:23
@@ -114,6 +114,7 @@
"karma-jasmine-sinon": "^1.0.4",
"lighthouse": "^1.6.3",
"moxios": "^0.4.0",
"prop-types": "^15.5.10",
Copy link
Contributor

Choose a reason for hiding this comment

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

would this be a general dependency and not just a dev dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, prop types are only checked locally, they don't have any bearing on production code.

Copy link
Contributor

@jcscottiii jcscottiii left a comment

Choose a reason for hiding this comment

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

Any reason to not move this line to use at least 15.5? https://github.com/18F/cg-dashboard/pull/1150/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R97 (i know the shrinkwrap uses 15.5.4

@el-mapache
Copy link
Contributor Author

el-mapache commented Jul 5, 2017

Sorry, I'm not sure what you mean 😞 . The package.json file is reporting the prop-types version is 15.5, do you mean I should update react to a newer version?

Copy link
Contributor

@msecret msecret left a comment

Choose a reason for hiding this comment

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

This looks good. I'm not sure we'd need to update react right now.

Only thing, this conflicts with a new PR of mine: #1151. Could we run the react codemon on that?

@jcscottiii
Copy link
Contributor

I was looking at the package.json
image
and the shrinkwrap has:
image

The whole reason I bring this up is because I checked codemod and I see this: https://github.com/reactjs/react-codemod#react-proptypes-to-prop-types
it needs 15.5.X for react for that particular one.

Luckily, the shrinkwrap already has us at 15.5.X but the package.json isn't but I'm thinking of the case where we go to regenerate the shrinkwrap and we get messed up with a 15.4.X version. (I could be thinking of an impossible case but I'm also coming a place where I would like to be sure our documentation for dependencies is up-to-date.)

Let me know your thoughts.

@msecret
Copy link
Contributor

msecret commented Jul 5, 2017

Oh I see what you mean now. That' actually kinda makes sense, because our package.json says use any version of react below v15. So we probably updated our deps and it updated shrinkwrap but doesn't have to update package.json.

@el-mapache
Copy link
Contributor Author

el-mapache commented Jul 6, 2017

@jcscottiii I think the '^' means that npm will try to update any minor or teeny versions of the package when it can, until the next major release. So, like @msecret said, when I regenerated the shrinkwrap file, npm saw there was a newer version of React (15.5.4) and picked that one over 15.4.2. Since it's a minor version there shouldn't be any breaking changes, and we should be safe to upgrade to that version of React when regenerating dependencies from the shrinkwrap file.

But, if we really want to have consistency, maybe we should peg the React version package.json and manually change it whenever we need to update React?

@jcscottiii
Copy link
Contributor

That's true.

We can leave it as-is. Let's revisit this part when @msecret revisits the shrinkwrap part and if even matters since the shrinkwrap already pegs it.

@jcscottiii jcscottiii merged commit 2ac776b into master Jul 6, 2017
@jcscottiii jcscottiii deleted the ab-add-prop-types-package branch July 6, 2017 17:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants