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

feat: add android clean project #38

Merged
merged 2 commits into from
Jul 17, 2020

Conversation

wilau2
Copy link
Contributor

@wilau2 wilau2 commented Dec 4, 2019

No description provided.

@wilau2 wilau2 closed this Dec 5, 2019
@wilau2 wilau2 deleted the feat/add-android-clean-project branch December 5, 2019 00:52
@wilau2 wilau2 restored the feat/add-android-clean-project branch December 5, 2019 16:59
@mikehardy
Copy link
Contributor

Hey there! Out of curiosity, is this closed for a specific reason? The related issue is correct in that deleting android/build is not sufficient, it should be android/app/build but even then that is just convention, the 'app', does not have to be called 'app' so that folder might not exist, meaning ./gradlew clean is actually the right approach

so it seems like this would be a good change, in my opinion

@wilau2
Copy link
Contributor Author

wilau2 commented Jan 7, 2020

Hey @mikehardy this one is still open: #36

@mikehardy
Copy link
Contributor

@pmadruga near as I can tell, this is a good change and a correct implementation of what should be done to clean android. No idea why the PR is closed, I think it should be merged :-)

@pmadruga
Copy link
Owner

Sure @mikehardy. Just wanted to see the reason why it was closed (maybe the author found something fishy?).

@mikehardy
Copy link
Contributor

Maybe with the fresh comments @wilau2 will chime in :-)

@wilau2
Copy link
Contributor Author

wilau2 commented May 27, 2020

Hey guys,
I ended up creating scripts in packages.json directly. It was easier for me to modify. At this point in our project, this is what we have:

"clean": "npm run clean:ios && npm run clean:android && npm run clean:javascript && rm -rf node_modules",
"clean:android": "lerna exec -- 'if [ -d \"android\" ]; then (cd android && ./gradlew clean); fi'",
"clean:ios": " lerna exec -- rm -rf ios/build && lerna exec -- rm -rf ios/pods",
"clean:javascript": "lerna clean --yes && rm -rf $TMPDIR/react-* && rm -rf $TMPDIR/metro-*",

@mikehardy
Copy link
Contributor

Ah cool @wilau2 - thanks for responding. From my perspective then, this is still a desired react-native-clean-project feature, and if I were to implement it, I'd implement it exactly the same. Any reason not to keep the PR open and mere it for others to use so they don't have to do the package.json scripting if they don't want to? I use react-native-clean-project in my role as maintainer of a few repos so I can give people one command to clean out completely so it's valuable to me to have it all here in one spot

@wilau2
Copy link
Contributor Author

wilau2 commented May 27, 2020

I moved away and you can probably see my train of thoughts with the different PRs I closed:

image

In the end, I'd prefer a collection of reusable scripts than a dynamic client.
I needed to fight to make it command line friendly from our ci. Make it support npm instead of yarn and yadi yada. 🤷

This PR is probably good to merge, I was just blocked by other stuff I needed to modify and got lost in a lot of PRs at some point.

@pmadruga
Copy link
Owner

That's really interesting and thanks for sharing it. I wonder whether a simple Q&A would work? For example, when running the library for the first time, it would ask whether is android/ios, npm/yarn and then generate a file with the config options. Next time the script runs, it would read from that file.

@mikehardy
Copy link
Contributor

For me this project is specifically about wiping away corrupt state / caching in a deterministic never-fails way (so I can converge colleague's environments with one command, and know that CI / release builds work every time). So for it to have it's own state across runs is like giving a fish a bicycle :-), completely outside what I'm interested in - likely to add another set of quirks to the one tool I trust to de-quirk everything else

@wilau2
Copy link
Contributor Author

wilau2 commented Jun 15, 2020

Requirements:

  • CI working 100% of the time (needs to be CI command line friendly)
  • When you have 20 persons contributing on your app. When they have a problem you want all solutions to be: npm run clean && npm install

I prefer it to be long but have 100% success rate than customizable to save some time.

@pmadruga
Copy link
Owner

pmadruga commented Jul 1, 2020

That's great feedback, folks. Thank you for it.

I'll be looking into incorporating the PR into master.

PS: npm run clean already includes npm install. But now that I'm thinking, maybe it should not include npm install (hence avoiding the whole yarn VS npm issue).

@pmadruga pmadruga reopened this Jul 17, 2020
@pmadruga pmadruga merged commit fd53ac8 into pmadruga:master Jul 17, 2020
@mikehardy
Copy link
Contributor

sorry for the very late feedback (but thank you for merging this!) - you mention:

PS: npm run clean already includes npm install. But now that I'm thinking, maybe it should not include npm install (hence avoiding the whole yarn VS npm issue).

I have a preference for "everything by default" but "everything with a switch" as stated before - just a personal preference, I understand people are different - but based on that, having it be one thing that "cleans node_modules" and one separate thing "installs node_modules" would be more towards my liking.

A minor point for me though, I still recommend this project about 2x a day :-) - it has 265 stars now too, pretty cool I think. Cheers

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.

3 participants