From fee9547ebc6a1d5860751285d503b1b38e221c56 Mon Sep 17 00:00:00 2001
From: Anton Gilgur <agilgur5@gmail.com>
Date: Mon, 20 Jun 2022 18:37:30 -0400
Subject: [PATCH] refactor(cache): simplify `clean` method (#358)

* refactor(cache): simplify `clean` method

- `cache().clean()` is only called during instantiation in `options`, so we can instead just call it inside of the constructor
- there is no need to call `this.init()` in `clean` as it's only used in the constructor anyway, which already calls `this.init()`
  - and if `noCache` is `true`, `init` is basically a no-op too
    - so technically we don't need to call `init` _at all_ if `noCache`, but that's a larger refactor that I'm splitting into a separate commit/PR

- now that `clean` is just one conditional, we can invert it and return early instead
- it's also not necessary and less efficient to call `emptyDir` before `remove`; `remove` will unlink all the contents anyway
  - docs here: https://github.com/jprichardson/node-fs-extra/blob/0220eac966d7d6b9a595d69b1242ab8a397fba7f/docs/remove-sync.md
    - though this also just normal FS behavior
- IMO, it's also not necessary to check if it's a directory, we can `remove` it either way
  - and not necessary to log out if we _don't_ clean it
- then also just simplify the logic to use a `filter` instead of a nested `if`
  - which we already do in several places, so this follows existing code style

* undo dir and not clean log removal

- as requested in code review, will go with a safety-first operating assumption

- as such, with safety as modus operandus, make the logging more detailed in these scenarios, since they're not supposed to happen
  - and don't check code coverage on these as they're not supposed to happen in normal usage
---
 src/index.ts   |  3 ---
 src/tscache.ts | 47 ++++++++++++++++++++++++++++-------------------
 2 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/src/index.ts b/src/index.ts
index 20c0e98..dccd133 100644
--- a/src/index.ts
+++ b/src/index.ts
@@ -128,9 +128,6 @@ const typescript: PluginImpl<RPT2Options> = (options) =>
 					noErrors = false;
 			}
 
-			if (pluginOptions.clean)
-				cache().clean();
-
 			return config;
 		},
 
diff --git a/src/tscache.ts b/src/tscache.ts
index 49dc7b2..a9724d4 100644
--- a/src/tscache.ts
+++ b/src/tscache.ts
@@ -1,5 +1,5 @@
 import * as tsTypes from "typescript";
-import { emptyDirSync, pathExistsSync, readdirSync, removeSync, statSync } from "fs-extra";
+import * as fs from "fs-extra";
 import * as _ from "lodash";
 import { Graph, alg } from "graphlib";
 import objHash from "object-hash";
@@ -127,6 +127,8 @@ export class TsCache
 				},
 				this.hashOptions,
 			)}`;
+		} else {
+			this.clean();
 		}
 
 		this.dependencyTree = new Graph({ directed: true });
@@ -146,26 +148,33 @@ export class TsCache
 		this.checkAmbientTypes();
 	}
 
-	public clean()
+	private clean()
 	{
-		if (pathExistsSync(this.cacheRoot))
-		{
-			const entries = readdirSync(this.cacheRoot);
-			entries.forEach((e) =>
-			{
-				const dir = `${this.cacheRoot}/${e}`;
-				if (e.startsWith(this.cachePrefix) && statSync(dir).isDirectory)
-				{
-					this.context.info(blue(`cleaning cache: ${dir}`));
-					emptyDirSync(`${dir}`);
-					removeSync(`${dir}`);
-				}
-				else
-					this.context.debug(`not cleaning ${dir}`);
-			});
-		}
+		if (!fs.pathExistsSync(this.cacheRoot))
+			return;
 
-		this.init();
+		const entries = fs.readdirSync(this.cacheRoot);
+		entries.forEach((e) =>
+		{
+			const dir = `${this.cacheRoot}/${e}`;
+
+			/* istanbul ignore if -- this is a safety check, but shouldn't happen when using a dedicated cache dir */
+			if (!e.startsWith(this.cachePrefix))
+			{
+				this.context.debug(`skipping cleaning ${dir} as it does not have prefix ${this.cachePrefix}`);
+				return;
+			}
+
+			/* istanbul ignore if -- this is a safety check, but should never happen in normal usage */
+			if (!fs.statSync(dir).isDirectory)
+			{
+				this.context.debug(`skipping cleaning ${dir} as it is not a directory`);
+				return;
+			}
+
+			this.context.info(blue(`cleaning cache: ${dir}`));
+			fs.removeSync(`${dir}`);
+		});
 	}
 
 	public setDependency(importee: string, importer: string): void