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.

Sign up for GitHub

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

Jump to bottom

Convert React.createClass to React.Component #877

Merged
merged 1 commit into from
Apr 10, 2017
Merged

Convert React.createClass to React.Component #877

merged 1 commit into from
Apr 10, 2017

Conversation

koba04
Copy link
Contributor

@koba04 koba04 commented Apr 10, 2017

In [email protected], React.createClass is deprecated, So we have to use create-react-class instead. #876

In this PR, I've converted React.createClass to React.Component except some tests for React.createClass.
Should Enzyme support create-react-class? If it isn't necessary, I can remove all React.createClass from the tests.

After this PR is merged, Enzyme can put create-react-class in devDependencies instead of dependencies.

@koba04 koba04 mentioned this pull request Apr 10, 2017
Merged
ljharb
ljharb requested changes Apr 10, 2017
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Please revert all the non-docs changes, and then this can land in master. #876 doesn't change the need to test, forever, createClass classes, so tests should continue to test them.

src/ReactWrapperComponent.jsx

getInitialState() {
return {
class WrapperComponent extends React.Component {
Copy link
Member

Choose a reason for hiding this comment

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

we definitely shouldn't change the specs here; we should continue to use createClass.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ljharb What's the reason to continue to use createClass? It seems to be able to replace with ES2015 classes. Is there any problems?

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 it would be nice if Enzyme users don't have to install create-react-class to use it.

Copy link
Member

Choose a reason for hiding this comment

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

oops, you're right, this one isn't a spec file - this can stay as-is :-)

nfcampos
nfcampos reviewed Apr 10, 2017
View reviewed changes
Copy link
Collaborator

@nfcampos nfcampos left a comment

Choose a reason for hiding this comment

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

i'd you undo the changes to the test files this can be merged

docs/api/ShallowWrapper/matchesElement.md Outdated
@@ -21,16 +21,16 @@ render tree.


```jsx
const MyComponent = React.createClass({
class MyComponent extends React.Component {
handleClick() {
...
Copy link
Collaborator

Choose a reason for hiding this comment

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

this component doesn't really work now that it's a class because methods aren't auto bound, so if this is used in handleClick it won't work.

do we want to have an "invalid" component as an example in the docs?

Copy link
Member

Choose a reason for hiding this comment

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

good catch - let's add the best practice of a manual constructor binding to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I've fixed it.

@koba04 koba04 force-pushed the convert-es2015-class branch from 0056359 to a9c7b66 Compare April 10, 2017 06:23
@koba04 koba04 force-pushed the convert-es2015-class branch from a9c7b66 to 0dfdd84 Compare April 10, 2017 06:40
@koba04
Copy link
Contributor Author

koba04 commented Apr 10, 2017

Ok, I've reverted changes for tests.

But I think it's good to use React.createClass only in tests that React.createClass is required because React is going to separate React.createClass from React core.

@nfcampos
Copy link
Collaborator

LGTM

nfcampos added a commit to kentcdodds/enzyme that referenced this pull request Apr 10, 2017
@nfcampos
Remove dependency on create-react-class
3a24258 
- remove createClass export from src/react-compat
- add test/react-compat with createClass export
- change spec files to import createClass from test/react-compat
- undo changes to src/ReactWrapperComponent.jsx (use of createClass there removed in enzymejs#877
- update README with correct dependencies for [email protected]
@ljharb ljharb merged commit c8f2185 into enzymejs:master Apr 10, 2017
nfcampos added a commit to kentcdodds/enzyme that referenced this pull request Apr 10, 2017
@nfcampos
Remove dependency on create-react-class
107e2e6 
- remove createClass export from src/react-compat
- add test/react-compat with createClass export
- change spec files to import createClass from test/react-compat
- undo changes to src/ReactWrapperComponent.jsx (use of createClass there removed in enzymejs#877
- update README with correct dependencies for [email protected]
@koba04
Copy link
Contributor Author

koba04 commented Apr 10, 2017

Thanks!

@koba04 koba04 deleted the convert-es2015-class branch April 10, 2017 09:48
nfcampos added a commit to kentcdodds/enzyme that referenced this pull request Apr 10, 2017
@nfcampos
Remove dependency on create-react-class
403225d 
- remove createClass export from src/react-compat
- add test/react-compat with createClass export
- change spec files to import createClass from test/react-compat
- undo changes to src/ReactWrapperComponent.jsx (use of createClass there removed in enzymejs#877
- update README with correct dependencies for [email protected]
ljharb pushed a commit to kentcdodds/enzyme that referenced this pull request Apr 12, 2017
@nfcampos @ljharb
Remove dependency on create-react-class
1fafc7f 
- remove createClass export from src/react-compat
- add test/react-compat with createClass export
- change spec files to import createClass from test/react-compat
- undo changes to src/ReactWrapperComponent.jsx (use of createClass there removed in enzymejs#877
- update README with correct dependencies for [email protected]
ljharb pushed a commit to kentcdodds/enzyme that referenced this pull request Apr 12, 2017
@nfcampos @ljharb
Remove dependency on create-react-class
edeb99c 
- remove createClass export from src/react-compat
- add test/react-compat with createClass export
- change spec files to import createClass from test/react-compat
- undo changes to src/ReactWrapperComponent.jsx (use of createClass there removed in enzymejs#877
- update README with correct dependencies for [email protected]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nfcampos nfcampos nfcampos approved these changes

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@koba04 @nfcampos @ljharb