-
-
Notifications
You must be signed in to change notification settings - Fork 52
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
Refactor for plugin capability, add pods and npm clean commands #12
Conversation
Thanks for this and yes, it can totally be used as a plugin. I will only ask for retroactive compatibility and any changes requested that may have come from the review. Jumping to the review 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.
Looks good to me. Left a few comments. Somethings are mandatory:
- A README update
- Writing a test ensuring a specific task has been called (you can use Tapejs for this)
And beware that the version of node on this project is slightly older (=>8*) than the one in React Native's documentation.
👍
Oh great - I'm sure we can get this in a shape you like then. I'm pretty agnostic on everything except that things should work and be easy to maintain :-). I'll go through your review now |
Worth mentioning that I agree 100% on backwards compatibility, I see no reason this should be a breaking change in any way, it's really just making the code so it can be wrapped, then doing so. I think the community will really like this - they were excited when I showed them this package, since the UI and feedback is really cool for users |
…nore example project
- refactor original implementation into separate internal modules - re-package implementation for use as react-native-community/cli plugin (v1 and v2) - turn contents section in README into a table, describe when tasks run - change spawn to use shell: true so we can group tasks - alter watchman task to add '|| true' so it does not return an error - add test to verify current tasks run in plugin auto mode Fixes #12 / Fixes react-native-community/cli#387
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.
Looks good 👍 Left some comments. Also, I'll create a dist
version of this lib in the future so I also left a comment to make sure I can just edit it one/two places.
@@ -119,7 +119,7 @@ that are not aligned to this Code of Conduct, or to ban temporarily or | |||
permanently any contributor for other behaviors that they deem inappropriate, | |||
threatening, offensive, or harmful. | |||
|
|||
### Scope | |||
### Code of Conduct Scope |
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.
Why this change?
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.
This was markdown lint - there were two headings with identical contents, so generated header anchors won't work. So I just changed one slightly. It's just lint so I don't really care, but it's a valid technical change - if it's a poor stylistic change, easy to revert. What do you think?
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.
Looks good. Thank you!
const { executeTask } = require('./internals/executor'); | ||
const { tasks } = require('./internals/tasks'); | ||
|
||
options.askiOS() |
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 the askiOS should be separated from the askAndroid task, opposed to the latter being included in the first. It will allow some concern separation in the short term (e.g., the askiOS shouldn't run tasks.wipeAndroidBuildFolder) and also more scalable in the future, in case we then want to really add OS specific tasks.
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.
Okay - let me think about that one (to do it well) and address 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.
Hmm - on review I think this is original structure as well - they were mixed to start - and is orthogonal to my changes. I need to draw boundaries or I'll be on this forever. If it was simple I'd go after it immediately but really it should be two phases fully separate / one for all asking, and one for all execution. It's sort of like that now but if you want to disentangle it's really going to be up to your taste and there is a large chance I won't do it the way you'll want to live with. Additionally there already were OS-specific tasks, but they weren't handled that way to begin with (brew is macOS only, pods are new and macOS only so I just followed the pattern of brew, watchman isn't on linux but is on windows and mac, there's no "choco" but that is recommended for windows etc, also some people use yarn some use npm, the package install should check for yarn.lock or package-lock.json). So there's lots of scope here to do it really right, but I followed existing pattern. I'd like to leave this for now if possible?
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're right and I totally agree to leave it for now. I'll get to it in the future then.
(I hope my comment before didn't sound as criticism)
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.
:-), no worries - I love a good engineering debate, it makes great software. Just running out of time and wanted to fix the scope out of self-interest haha
console.log('Use \`./node_modules/.bin/react-native-clean-project\' for total control'); | ||
console.log(''); | ||
|
||
rlInterface.close(); // if we don't do this it hangs waiting for input |
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.
The tests' results also raise a warning regarding an open task that's keeping preventing jest from exiting, and it's related to rlInterface
. You can check it by running ./node_modules/.bin/jest --detectOpenHandles
. Can you give it a look, please?
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 gave that one a long look and I'm not sure how to fix it well. My general take was that it was like that before - by which I mean structurally you have the readline opened immediately whether it will be used or not, then you close it in an unrelated area. My conversion to a plugin style didn't relate to that though it did mean I had to be careful to close it myself in plugin.js. Unfortunately even though I'm closing it there it isn't closing when run by jest.
It was late enough at night and I'd worked enough on it that after contemplating the "no fix" path (no real consequence) I let it go. It's an existing issue with the code that needs cleaning but it'll require re-organizing how readline is used in the options.js which are orthogonal to my change.
Can we let it go for this PR?
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 also have a point here and I'll take it in another PR. Thank you for explaining it thoroughly.
Thanks so much for the quick review, great comments and I'll fix what's obviously wrong while waiting for your reply on the rest. |
Okay - I've addressed all the comments, even if for the 2 more substantial ones I was essentially pleading to keep them separate. Let me know what you think - thanks! |
Thanks for the replies, @mikehardy . The PR looks great and exposed some of the issues that I need to address. Thumbs up from me. I'll merge as soon as possible - would just like @thymikee 's thumbs up. And overall, thanks for this PR. |
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.
💯
Fantastic! I'm not part of the CLI team (I just run around making changes on things that drive me crazy personally) but I am going to use the heck out of this thing now. If the CLI folks want to keep this out of core I could see this plugin being the de facto standard at which point you and your module will be #InternetFamous, which is fun :-). Cheers |
Co-Authored-By: Michał Pierzchała <thymikee@gmail.com>
@thymikee I'm so glad you looked at this, I would have never known that helped the CLI, not knowing the internals. I committed your suggestion exactly as is |
@pmadruga Any chance of a release with this soon 🙏 😃 - would love to recommend this as a simple npm install vs using a git master dependency |
Done :) |
Hi there!
A few of us in the react-native-community were discussing how we could never escape the need to completely wipe out state and start clean, and the react-native command line should just have a command for it.
The CLI maintainers mentioned we could probably do it as a plugin pretty quickly, and I found this project, and thought you might be okay with refactoring it so it could be used as a plugin?
So I did that, and I also added a couple extra possible commands - pod delete, pod update, and npm cache clean
It should work with both CLI 1.x and the upcoming 2.x for react-native 0.60, and you just need to do
react-native clean-project
orreact-native clean-project-auto
to have it either take you through the questions, or just purge everything respectivelyIf it's merge-worthy, it fixes react-native-community/cli#387