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

Make compatible with react 16 #90

Closed

Conversation

HelveticaScenario
Copy link

createClass and PropTypes were moved into separate packages in react 16. this should not break compatibility with react 15

@@ -746,11 +746,24 @@
if (typeof exports === 'object' && typeof module !== 'undefined') {
var React = require('react'); // eslint-disable-line no-undef
var ReactDOM = require('react-dom'); // eslint-disable-line no-undef
if (!React.createClass) {
React.createClass = require('create-react-class'); // eslint-disable-line no-undef
Copy link
Owner

Choose a reason for hiding this comment

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

Rather than mutating React It'd be better to pass either React.PropTypes or PropTypes into the getReorderComponent function. Same with createClass. :)

Copy link
Author

Choose a reason for hiding this comment

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

i agree, i was just trying to make as few changes as i could

Copy link
Author

Choose a reason for hiding this comment

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

to be fair, you shouldnt be using createClass or PropTypes at all, they have been deprecated.

Copy link
Author

Choose a reason for hiding this comment

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

you are already using babel, theres no reason you cant just use es6 classes

Copy link
Owner

Choose a reason for hiding this comment

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

I believe I first write this lib before ES6 was a standard and so I was avoiding it. I have since not migrated as due to the lack of auto-binding in ES6 classes I thought it would likely cause more bugs and not improve the library in any noticeable way.

Copy link
Author

@HelveticaScenario HelveticaScenario Oct 8, 2017

Choose a reason for hiding this comment

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

I understand, and I dont think the way you're doing it is particularly wrong, it just isnt idiomatic anymore. and there is a way of doing auto-binding in es6 classes, you do it like this.

class ASD {
  funcName = (arg) => {
    // ...
  }
}

I do think, however, that continuing to use deprecated features when more idiomatic alternatives exist probably isnt the best way of going about things. But its up to you.

Copy link
Owner

Choose a reason for hiding this comment

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

That was a proposed ES7 syntax as far as I know, and so I wouldn't like to use that until it's finalized. I am currently in the middle of a re-write though, so it is unlikely v2 will ever use classes.

@JakeSidSmith
Copy link
Owner

@HelveticaScenario I will probably take some time this weekend to convert this to use ES6 classes, so don't worry about updating this PR. :)

@JakeSidSmith
Copy link
Owner

Fixed these issues in alpha.6: #94

Gonna close this PR.

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.

None yet

2 participants