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 "clean" command #1582

Merged
merged 10 commits into from
Apr 7, 2022

Conversation

tido64
Copy link
Contributor

@tido64 tido64 commented Apr 1, 2022

Summary:

This adds a clean command to the CLI for common cleaning operations.

Originally contributed by @dennismunene: microsoft/rnx-kit#993

Test Plan:

yarn link @react-native-community/cli @react-native-community/cli-clean

Test --help:

% npx react-native clean --help
react-native clean

Cleans your project by removing React Native related caches and modules.

Options:
  --include <string>       Comma-separated flag of caches to clear e.g. `npm,yarn`. If omitted, an interactive prompt will appear.
  --project-root <string>  Root path to your React Native project. When not specified, defaults to current working directory. (default: "/~/test-app")
  --verify-cache           Whether to verify the cache. Currently only applies to npm cache.
  -h, --help               output usage information

By default, prompts the user for caches to remove:

% yarn react-native clean
? Select all caches to clean ›
Instructions:
    ↑/↓: Highlight option
    ←/→/[space]: Toggle selection
    a: Toggle all
    enter/return: Complete answer
◯   android (Android build caches, e.g. Gradle)
◯   cocoapods (CocoaPods cache)
◉   metro (Metro, haste-map caches)
◯   npm (`node_modules` folder in the current package, and optionally verify npm cache)
◉   watchman (Stop Watchman and delete its cache)
◯   yarn (Yarn cache)

Remove node_modules:

% npx react-native clean --include npm
✔ Remove node_modules

Clean Gradle cache and node_modules:

% npx react-native clean --include android,npm
✔ Clean Gradle cache
✔ Remove node_modules

@thymikee
Copy link
Member

thymikee commented Apr 1, 2022

I'm looking at the include flag and wonder – can we make it interactive, with checkboxes to tick, and provide --no-interactive mode so it works the same as now?

@tido64
Copy link
Contributor Author

tido64 commented Apr 1, 2022

I'm looking at the include flag and wonder – can we make it interactive, with checkboxes to tick, and provide --no-interactive mode so it works the same as now?

I made it prompt if you omit --include. Let me know what you think.

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

This works really well! The only thing I'm missing is tests. Would you mind providing some?

Comment on lines 103 to 104
const npm = os.platform() === 'win32' ? 'npm.cmd' : 'npm';
const yarn = os.platform() === 'win32' ? 'yarn.cmd' : 'yarn';
Copy link
Member

Choose a reason for hiding this comment

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

In other packages, e.g. cli-doctor we use the execa package to handle this for us. Mind verifying it's ok and remove the platform checks? Hope this makes it unnecessary for the execute helper to exist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Testing this on Windows now. Will reply once complete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works on Windows. I also hid the CocoaPods option when on Windows and added hints:

% yarn react-native clean
? Select all caches to clean ›
Instructions:
    ↑/↓: Highlight option
    ←/→/[space]: Toggle selection
    a: Toggle all
    enter/return: Complete answer
◯   android (Android build caches, e.g. Gradle)
◯   cocoapods (CocoaPods cache)
◉   metro (Metro, haste-map caches)
◯   npm (`node_modules` folder in the current package, and optionally verify npm cache)
◉   watchman (Stop Watchman and delete its cache)
◯   yarn (Yarn cache)

@thymikee
Copy link
Member

thymikee commented Apr 6, 2022

After @satya164 comment internally:

one things I'd suggest is maybe some hint for npm and yarn cache? we rarely need to clean those but I see people clearing them all the time because of metro cache issue and they saw it somewhere

Can we uncheck yarn and npm by default? Cache issues are usually related to metro and watchman in my experience as well.

name: 'caches',
message: 'Select all caches to clean',
choices: Object.entries(groups).map(([cmd, group]) => ({
title: `${cmd} (${group.description})`,
Copy link
Member

Choose a reason for hiding this comment

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

how about dimming the description, so it doesn't clutter the output too much?

Suggested change
title: `${cmd} (${group.description})`,
title: `${cmd} ${chalk.dim((${group.description}))}`,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Yeah, that looks better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And on Windows:
image

@thymikee thymikee self-requested a review April 7, 2022 09:37
Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Beauty!

@@ -1,5 +1,6 @@
import {getLoader} from '@react-native-community/cli-tools';
import type {Config as CLIConfig} from '@react-native-community/cli-types';
import chalk from 'chalk';
Copy link
Member

@thymikee thymikee Apr 7, 2022

Choose a reason for hiding this comment

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

we'll need to add chalk to package.json. Hope this fixes the CI that started to fail with weird unrelated error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops 😄

@thymikee thymikee merged commit ada9516 into react-native-community:master Apr 7, 2022
@tido64 tido64 deleted the tido/clean-command branch April 7, 2022 09:45
const script = path.basename(gradlew);
await execa(
os.platform() === 'win32' ? script : `./${script}`,
['clean'],
Copy link
Member

Choose a reason for hiding this comment

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

Small FYI: clean won't work for users on New Architecture due to a bug on AGP that is causing clean to depend on preBuild. I agree that ./gradlew clean would be the correct approach here, but maybe we should not advertise it yet? Or have a isNewArchitectureEnabled flag inside the CLI to check this and run a rm?

@adamTrz adamTrz mentioned this pull request Nov 18, 2022
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