-
Notifications
You must be signed in to change notification settings - Fork 931
Updated documentation to make contributing a tiny bit easier #806
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
Updated documentation to make contributing a tiny bit easier #806
Conversation
thymikee
left a comment
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.
Thank you for your efforts, this is much appreciated! If running the CLI directly doesn't work (I remember using it for init, which doesn't interoperate with platform-related packages), maybe we should remove it, or only show it for init?
| yarn link "@react-native-community/cli-platform-android" | ||
|
|
||
| npx react-native run-android | ||
| ``` |
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.
You can see this gets pretty complicated and only affects a single package. We need an npm-script that would cd to every package and run yarn link there, so that folks can then run:
yarn link @react-native-community/cli-tools @react-native-community/cli-platform-ios @react-native-community/cli-platform-android @react-native-community/cli @react-native-community/cli-types
Then we could write something like:
Using yarn link
CLI is a monorepo with multiple packages interacting with each-other. In order to effortlessly test your changes you'll need to "link" them using yarn link for every package. You can do so with this command:
yarn link-packages
And then, in your test project, run:
yarn link @react-native-community/cli-tools @react-native-community/cli-platform-ios @react-native-community/cli-platform-android @react-native-community/cli @react-native-community/cli-types
From now on, your test project will transparently use the CLI from symlinked packages.
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.
Yeah, I remember running into it when I did link/cli and ended up not seeing my Android changes.
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.
Ok, I'll work on that !
CONTRIBUTING.md
Outdated
|
|
||
| ## Testing your changes | ||
|
|
||
| ### Directly |
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 would get rid of this setup entirely. I think it's potentially confusing. When you run the CLI directly from a different folder, you're still going to be loading "platform-android" and "platform-ios" packages from the "node_modules" of your project.
This is because CLI itself doesn't load Android/iOS specific commands anymore. It finds React Native dependency and that's the package that depends on cli-platform-android and cli-platform-ios.
So in order to see your changes in these two, you'd have to link them anyway.
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 as I've run into this issue myself right now haha)
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.
Yes please. I think it only makes sense for detached commands like init
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.
Sounds good, I'll remove this part then :)
55847d5 to
75b228f
Compare
|
Updated the PR! |
package.json
Outdated
| "test:ci:cocoapods": "ruby packages/platform-ios/native_modules.rb", | ||
| "postinstall": "yarn build", | ||
| "link-packages": "PACKAGES=('tools' 'platform-ios' 'platform-android' 'cli' 'cli-types' 'global-cli') && cd packages && for PACKAGE in \"${PACKAGES[@]}\"; do cd $PACKAGE && yarn link && cd ..; done", | ||
| "unlink-packages": "PACKAGES=('tools' 'platform-ios' 'platform-android' 'cli' 'cli-types' 'global-cli') && cd packages && for PACKAGE in \"${PACKAGES[@]}\"; do cd $PACKAGE && yarn unlink && cd ..; done", |
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 don't see this necessary. yarn link only marks a dependency as "linkable", no need to unlink these. Also, please remove global-cli here and in link-packages, we don't want anybody to touch it for now.
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.
Could be useful in a very few cases where you have several clone of the cli on your machine? 😅 Ok, very small chance.
Do you still want me to remove it? 🙂
thymikee
left a comment
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.
Almost there :)
Co-Authored-By: Michał Pierzchała <thymikee@gmail.com>
Co-Authored-By: Michał Pierzchała <thymikee@gmail.com>
Summary:
I had a little bit of a hard time testing my changes, because I didn't have the reflex to look for the
CONTRIBUTING.mdfile.Also, not sure why, but the method described to
test your changes directlydoes not seem to work for me, nothing happens, so I added the way to test it usingyarn linkTest Plan:
This is documentation only :)