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

Remove create-react-class dependency #23724

Closed
wants to merge 12 commits into from

Conversation

ericlewis
Copy link
Contributor

@ericlewis ericlewis commented Mar 1, 2019

Summary

Remove create-react-class from the core of RN. It is only used in a few components, and removes it from package.json as well. This saves us 4.19mb after install thanks to the removal of conflicting packages.

Note: it uses the library react-mixin, it is quite tiny. I am open to other ideas of course.

Changelog

[General] [Removed] - remove create-react-class dependency from core

Test Plan

Interact with the RNTester app, let CI do its thang.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 1, 2019
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • flow found some issues.

Libraries/Components/TextInput/TextInput.js Show resolved Hide resolved
Libraries/Components/TextInput/TextInput.js Outdated Show resolved Hide resolved
Libraries/Components/TextInput/TextInput.js Show resolved Hide resolved
Libraries/Components/TextInput/TextInput.js Show resolved Hide resolved
Libraries/Components/TextInput/TextInput.js Outdated Show resolved Hide resolved
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • flow found some issues.

Libraries/Components/TextInput/TextInput.js Outdated Show resolved Hide resolved
Libraries/Components/TextInput/TextInput.js Show resolved Hide resolved
Libraries/Components/TextInput/TextInput.js Show resolved Hide resolved
@pull-bot
Copy link

pull-bot commented Mar 2, 2019

Warnings
⚠️ 🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against fdaaf94

Need to be able to run tests on components that haven’t been converted.
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • flow found some issues.

Libraries/Components/TextInput/TextInput.js Show resolved Hide resolved
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • flow found some issues.
  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

Libraries/Components/TextInput/TextInput.js Show resolved Hide resolved
Libraries/Components/TextInput/TextInput.js Outdated Show resolved Hide resolved
Libraries/Components/TextInput/TextInput.js Outdated Show resolved Hide resolved
Libraries/Components/TextInput/TextInput.js Outdated Show resolved Hide resolved
Libraries/Components/Touchable/TouchableWithoutFeedback.js Outdated Show resolved Hide resolved
Libraries/Components/Touchable/TouchableWithoutFeedback.js Outdated Show resolved Hide resolved
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • flow found some issues.
  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • flow found some issues.
  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

Libraries/Components/TextInput/TextInput.js Show resolved Hide resolved
Libraries/Components/TextInput/TextInput.js Outdated Show resolved Hide resolved
Libraries/Components/TextInput/TextInput.js Outdated Show resolved Hide resolved
Libraries/Components/TextInput/TextInput.js Outdated Show resolved Hide resolved
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • flow found some issues.

@ericlewis ericlewis marked this pull request as ready for review March 2, 2019 14:59
@ericlewis ericlewis changed the title [WIP] Remove create-react-class dependency Remove create-react-class dependency Mar 2, 2019
@ericlewis
Copy link
Contributor Author

the android error is unrelated.

@elicwhite
Copy link
Member

I think we would rather switch these from using mixins at all. This is what we did for all the other components in the codebase. See #21581 and #21485 for details.

In order to do this we need to migrate off of TouchableMixin and convert it to something else. We've done this work internally and have been testing it. I believe moving it to open source is part of @cpojer's roadmap.

I'm pretty hesitant to switching to use a personally scoped package in the mean time. However, I think moving connect to devDeps is reasonable to do in a standalone PR since it looks like nothing uses it as a regular dependency.

@ericlewis
Copy link
Contributor Author

@TheSavior the personally scoped package is based on another package, with the polyfills removed. I’m happy to use the “supported” package. But it is quite old, (and the polyfills that aren’t needed ofc).

I’ll take a look at your examples and see if I can adapt to using them instead of the package. Will also open a sep PR for connect.

@elicwhite
Copy link
Member

Do you have measurements on what the bundle size impact would be if the Touchables (and thus createreactclass) were pulled out from React Native?

@ericlewis
Copy link
Contributor Author

@TheSavior ~5MB saved when installed with —production if we remove both the dep I introduced & the create-react-class. Honestly, it almost seems worth it to remove.. esp with fabrics touch events coming on views.

@ericlewis
Copy link
Contributor Author

I used cost-of-modules to determine this.

@ericlewis
Copy link
Contributor Author

Oh, I’m sorry- bundle size is a different thing yes?

@elicwhite
Copy link
Member

Saves 5 MB where? In the node_modules folder?

If you create a production bundle with metro for an app created with React-Native init both before and after this change, what is the size of the bundle? I doubt the before bundle is 5MB so I’m not sure what the win is here to the number that really matters.

I think we should fix these files and get them off createReactClass but the urgency is different depending on the bundle size impact.

@cpojer
Copy link
Contributor

cpojer commented Mar 10, 2019

I think saving 5 mb of dev dependencies itself is worth it. However, my hesitation here comes from changing these modules. I’m worried about introducing subtle breakages just to reduce size. I agree with Eli that the right move is to deprecate some stuff and remove these modules altogether.

That being said, I think it’s totally worth removing the mixin and createReactClass from TextInput. Could you try to find a way to do this safely and without introducing new dependencies?

@cpojer cpojer closed this Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants