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

Migrated deprecated React.PropTypes and React.createClass #26

Merged
merged 1 commit into from
Apr 17, 2017

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Apr 8, 2017

I think we should migrate to the es6 class syntax and stop using React.createClass, but that would be another step. For now just created this PR, so people stop having deprecation warnings after migration to React v15.5.

If you feel the same about class, I'll create an issue about it, so the matter won't get forgotten and neglected.

Copy link
Owner

@nkbt nkbt left a comment

Choose a reason for hiding this comment

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

Lgtm

@@ -1,4 +1,5 @@
import React from 'react';
import createReactClass from 'create-react-class';
import {shouldComponentUpdate} from 'react/lib/ReactComponentWithPureRenderMixin';
Copy link

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well, when we switch to es6 classes we just need to use PureComponent, so its easily fixable

Copy link
Owner

Choose a reason for hiding this comment

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

Just in case... If the only use of react-height for you is react-collapse - switch to react-collapse@3, that one does not have react-height dependency anymore.

Copy link
Contributor Author

@Andarist Andarist Apr 15, 2017

Choose a reason for hiding this comment

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

so this goal - "used as backend for react-collapse" should be removed?

also - what about merging this in? should something more be done here?

Copy link
Owner

Choose a reason for hiding this comment

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

I cannot see any deprecation notices about this. Which version of react we are talking here? 15.x still has it

Copy link

Choose a reason for hiding this comment

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

Deprecation on React v15.4.

However, there is a possibility that you imported private APIs from react/lib/*, or that a package you rely on might use them. We would like to remind you that this was never supported, and that your apps should not rely on internal APIs. The React internals will keep changing as we work to make React better.
-- React v15.4.0 - React Blog

I've test react-height on react v16.

npm i -D react@next

react/lib/ReactComponentWithPureRenderMixin has been removed.
Because, React v16 switch to flat bundles.

Switch to flat bundles (no more react-dom/lib/*, internals are truly private)
-- facebook/react#8854

package 2 2017-04-18 09-39-33

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, that is cool. Won't mind a PR for this. Currently it does not produce any warnings.

There was a very practical reason to deeplink it.

React team did not create a standalone package for shallow compare so we could not make it external. If we used special package - it just required this deeplinked react/lib/ReactComponentWithPureRenderMixin. So when included in the bundle - well... You include half of React package with it. Not nice for a small library.

Bundle size should not be increased when we migrated to whatever other solution is there...

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 think PureComponent is exactly this - Component + PureRenderMixin

Copy link
Owner

Choose a reason for hiding this comment

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

Can only be used with ES6 Classes, not createReactClass factory =(

Need to update all the code to use Classes instead, to avoid extra dependency.

@codecov
Copy link

codecov bot commented Apr 15, 2017

Codecov Report

Merging #26 into master will increase coverage by 5.76%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #26      +/-   ##
=======================================
+ Coverage   19.23%   25%   +5.76%     
=======================================
  Files           1     1              
  Lines          26    28       +2     
=======================================
+ Hits            5     7       +2     
  Misses         21    21
Impacted Files Coverage Δ
src/ReactHeight.js 25% <100%> (+5.76%) ⬆️

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 cd4d15f...8a59c09. Read the comment docs.

@Andarist
Copy link
Contributor Author

Andarist commented Apr 18, 2017 via email

@nkbt
Copy link
Owner

nkbt commented Apr 18, 2017

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