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

Use packages to prevent React v15.5.0 and up from complaining about deprecated API's #38

Merged
merged 3 commits into from
Apr 12, 2017

Conversation

JaapRood
Copy link
Contributor

To prevent React>=15.5.0 from throwing warnings we can't use React.createClass or React.PropTypes directly. For both there are now separate packages on npm. In case of createClass the recommendation is to convert to a ES6 class, however that's a bit more of an involved change. Instead, I reckon we can get this version out and silence those warnings first and then tackle a conversion to ES6 class separately.

Note: linter and all tests pass, but a warning is thrown by React during tests about calling a used PropType directly. However, I can't seem to figure out why, so it's probably caused by react-side-effect?

@JaapRood JaapRood changed the title Use and packages to prevent React v15.5.0 and up from complaining about deprecated API's Use packages to prevent React v15.5.0 and up from complaining about deprecated API's Apr 10, 2017
package.json Outdated
@@ -34,6 +34,8 @@
"react": "^0.13.0"
},
"dependencies": {
"create-react-class": "^15.5.1",
"prop-types": "^15.5.6",

Choose a reason for hiding this comment

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

Shouldn't it be in peerDependencies ?

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'm not sure. I guess you would pull in react as a peer dependency as users would actively want to provide their own copy of it, but is that the case with these two? react they definitely would have installed anyway, but might not use prop-types or create-react-class, which they then have to install manually and include in their project dependencies (as it goes with peerDependencies). Can you explain why you think they should be peerDependencies?

Copy link

@avocadowastaken avocadowastaken Apr 11, 2017

Choose a reason for hiding this comment

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

but might not use prop-types or create-react-class

I guess it's true

Can you explain why you think they should be peerDependencies?

I just don't think that this lib is big enough to be "batteries included" type.


Anyways I guess @gaearon does not have time or ambitions to maintain this project, so it's time to move on.

Copy link
Owner

@gaearon gaearon Apr 11, 2017

Choose a reason for hiding this comment

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

No, it should be in dependencies.

I'm happy to add maintainers who won't change the API (I don't think it's worth changing it at this point to avoid churn in a very simple library). If you'd like to help with maintenance let me know.

Copy link
Owner

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Let's either introduce Babel here or rewrite this in prototype style (e.g. type Babel loose mode-like output by hand). I don't want this to depend on create-react-class.

@JaapRood
Copy link
Contributor Author

@gaearon fair enough on not wanting to rely on create-react-class. I'll probably have some time today to rewrite it as a prototype.

@JaapRood
Copy link
Contributor Author

@gaearon how about this? Seemed the simplest way to create the component prototypally (stateless functions won't work because returning null was added until React v15).

I also removed React.createClass calls in the tests, something I totally missed before. I assumed that relying on create-react-class as a dev dependency is alright.

@@ -27,13 +27,15 @@
},
"homepage": "https://github.com/gaearon/react-document-title",
"devDependencies": {
"create-react-class": "^15.5.2",
Copy link
Owner

Choose a reason for hiding this comment

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

We can remove this now, right?

Copy link
Owner

Choose a reason for hiding this comment

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

Ah nevermind, it's dev deps.

@gaearon gaearon merged commit d38386d into gaearon:master Apr 12, 2017
@gaearon
Copy link
Owner

gaearon commented Apr 12, 2017

LGTM

@gaearon
Copy link
Owner

gaearon commented Apr 12, 2017

Out in 2.0.3, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants