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

[MAJOR] Drop react-native support, update deps, swap to GH Actions #21

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

decompil3d
Copy link

React Native was an outdated and insecure dependency. We don't use it at all inside GoDaddy anymore, so support in this module is bound to deteriorate. This PR drops react-native support altogether, updates dependencies, and swaps from Travis to GitHub Actions.

package.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@decompil3d decompil3d removed the request for review from Swaagie December 30, 2020 19:00
@akazemier-godaddy
Copy link
Contributor

Instead of deleting code, you could just have bumped the dependency and that's it. I don't see a need to delete the react-native code as it should just function.

@decompil3d
Copy link
Author

@akazemier-godaddy bumping the dependency would require bumping react to 17 as well. Furthermore, we don't have any remaining use-cases for react-native inside GoDaddy, so I worry that the react-native support would get stale while possibly hindering future development on this module. As such, my inclination is to jettison it. If someone in the community would like to use this module with RN, the logic is still in the git history and it would not be too hard to re-add it in as long as they're willing to maintain.

@tarkatronic
Copy link

@decompil3d @akazemier-godaddy Can we get some movement on this? It would be great to get these security updates merged.

@rmarkins-godaddy rmarkins-godaddy requested review from msluther and removed request for indexzero February 3, 2022 21:33
@rmarkins-godaddy
Copy link

@decompil3d @akazemier-godaddy We are over a year out on this PR do we have a consensus on the direction of the package? Need to move forward to progress w/ react@17 updates on here as well. Also since we are bumping to react@17 I don't think there is a reason to drop react-native (from an OSS perspective vs. internal organizational purpose). However seeing as this package isn't used externally from us (about 2 downloads per week externally) I see no reason to keep this react-native around.

Copy link

@msluther msluther left a comment

Choose a reason for hiding this comment

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

We should at least add node@16, and maybe drop node@12 now

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Comment on lines +43 to +44
"react": "^16.14.0",
"react-dom": "^16.14.0"
Copy link

Choose a reason for hiding this comment

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

We'll have to get these updated at some point as well, but that can be another PR.

@msluther
Copy link

msluther commented Feb 3, 2022

Overall though, I'm okay with dropping RN support. Unless someone is going to sign up to maintain it and keep it working/useful there. We might be able to keep RN-support limping along just by bumping the version, but at some point it's going to atrophy and require more attention. If we want to do that, I'm fine, but it's going to be a call we have to make at some point to either remove support or fix issues in it.

@mswaagman-godaddy
Copy link

Hindsight I'd argue that merging react-native with web packages is an anti-pattern and the path forward should be a base package with platform specific implementations extending the base module.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

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.

9 participants