-
Notifications
You must be signed in to change notification settings - Fork 907
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
chore: remove "rnpm" #790
chore: remove "rnpm" #790
Conversation
e2e tests are also working now because |
On that note, we should add "e2e" tests for autolinking too. |
Co-Authored-By: Michał Pierzchała <thymikee@gmail.com>
Awesome! When you say that these commands are removed now, do you mean that they will throw an exception when called? It might be nice if the commands still existed but give a nice message and link to something. With all the older libraries with readmes that say to run link I can imagine people getting confused if those start erroring. FWIW, I haven’t read the code. I’m on mobile and it’s too big to scroll and look for those related changes. So hopefully this isn’t a stupid question :-) |
@TheSavior, I like this idea! I will bring these commands back and print an appropriate message |
I will send a follow-up PR with better assets support. Removing them from documentation now because we don't support it and that the documentation will change. This will be a non-breaking change. |
* @todo: Remove this once `rnpm` config is deprecated and all major RN libs are converted. | ||
*/ | ||
const haste = config.haste || { | ||
const haste = { | ||
providesModuleNodeModules: isPlatform ? [dependencyName] : [], | ||
platforms: Object.keys(config.platforms), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need to provide haste anymore since 0.61?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing it down and will send a follow-up PR to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. cc @acoates-ms this will likely affect RN for Windows. If you haven't adopted autolinking yet, please prioritize it if possible. This change will land with RN 0.62 or later
894be40
to
52ac627
Compare
* Remove legacy link * Clean up ios config * Remove install/uninstall * Update comments * Update test on iOS * Clean up iOS dependencies * Type iOS config and set proper return types * Update scheme * Update schema * Not all projects need native code * No assumptions on CocoaPods, different package managers might be used * Fix type errors * Update snapshots to reflect missing config properties * Void is not undefined * Clean up Android configuration * Update schema * Serious rnpm/documentation removals and cleaned up tests * Remove remaining props * Update snapshots for some of the tests * Fix last test - fixture was broken * Update docs/dependencies.md Co-Authored-By: Michał Pierzchała <thymikee@gmail.com> * Fix type for scriptPhases * Update to | undefined instead of elvis * Add deprecated commands * Add deprecation links * Move undefined to null * Add documentation back * clean up * chore * Update snapshots * And update unit test back to null
This PR removes all
rnpm
configuration files and related code. Everything was already deprecated and messages were being displayed.This will ship with the next React Native.
Removed:
link
unlink
install
uninstall
hooks
,params
andassets
(now taken care byautolinking
or not supported)linkConfig
+ extraneous config propertiesUpdated:
This implements react-native-community/discussions-and-proposals#96