More PR Feedback

Nick Pape · Nick Pape · commit 8d4a24533211 · 2018-06-27T19:22:18.000-07:00
diff --git a/apps/api-documenter/src/markdown/MarkdownDocumenter.ts b/apps/api-documenter/src/markdown/MarkdownDocumenter.ts
@@ -619,6 +619,6 @@ export class MarkdownDocumenter {
 
   private _deleteOldOutputFiles(): void {
     console.log('Deleting old output from ' + this._outputFolder);
-    FileSystem.emptyFolder(this._outputFolder);
+    FileSystem.ensureEmptyFolder(this._outputFolder);
   }
 }
diff --git a/apps/api-documenter/src/yaml/YamlDocumenter.ts b/apps/api-documenter/src/yaml/YamlDocumenter.ts
@@ -420,7 +420,7 @@ export class YamlDocumenter {
 
     FileSystem.writeFile(filePath, stringified, {
       convertLineEndings: NewlineKind.CrLf,
-      ensureFolder: true
+      ensureFolderExists: true
     });
 
     if (schema) {
@@ -581,6 +581,6 @@ export class YamlDocumenter {
 
   private _deleteOldOutputFiles(): void {
     console.log('Deleting old output from ' + this._outputFolder);
-    FileSystem.emptyFolder(this._outputFolder);
+    FileSystem.ensureEmptyFolder(this._outputFolder);
   }
 }
diff --git a/apps/api-extractor/src/extractor/Extractor.ts b/apps/api-extractor/src/extractor/Extractor.ts
@@ -309,7 +309,7 @@ export class Extractor {
 
       // Write the actual file
       FileSystem.writeFile(actualApiReviewPath, actualApiReviewContent, {
-        ensureFolder: true
+        ensureFolderExists: true
       });
 
       // Compare it against the expected file
diff --git a/apps/api-extractor/src/generators/ApiJsonGenerator.ts b/apps/api-extractor/src/generators/ApiJsonGenerator.ts
@@ -48,7 +48,7 @@ export class ApiJsonGenerator extends AstItemVisitor {
     this.visit(context.package, this.jsonOutput);
 
     // Write the output before validating the schema, so we can debug it
-    JsonFile.save(this.jsonOutput, reportFilename, { ensureFolder: true });
+    JsonFile.save(this.jsonOutput, reportFilename, { ensureFolderExists: true });
 
     // Validate that the output conforms to our JSON schema
     ApiJsonFile.jsonSchema.validateObjectWithCallback(this.jsonOutput, (errorInfo: IJsonSchemaErrorInfo) => {
diff --git a/apps/api-extractor/src/generators/dtsRollup/DtsRollupGenerator.ts b/apps/api-extractor/src/generators/dtsRollup/DtsRollupGenerator.ts
@@ -117,7 +117,7 @@ export class DtsRollupGenerator {
 
     FileSystem.writeFile(dtsFilename, indentedWriter.toString(), {
       convertLineEndings: NewlineKind.CrLf,
-      ensureFolder: true
+      ensureFolderExists: true
     });
   }
 
diff --git a/apps/rush-lib/src/api/ChangeFile.ts b/apps/rush-lib/src/api/ChangeFile.ts
@@ -66,7 +66,7 @@ export class ChangeFile {
   public writeSync(): void {
     const filePath: string = this.generatePath();
     JsonFile.save(this._changeFileData, filePath, {
-      ensureFolder: true
+      ensureFolderExists: true
     });
   }
 
diff --git a/apps/rush-lib/src/api/LastInstallFlag.ts b/apps/rush-lib/src/api/LastInstallFlag.ts
@@ -45,7 +45,7 @@ export class LastInstallFlag {
    */
   public create(): void {
     JsonFile.save(this._state, this._path, {
-      ensureFolder: true
+      ensureFolderExists: true
     });
   }
 
diff --git a/apps/rush-lib/src/api/test/LastInstallFlag.test.ts b/apps/rush-lib/src/api/test/LastInstallFlag.test.ts
@@ -7,11 +7,11 @@ const TEMP_DIR: string = path.join(__dirname, 'temp');
 
 describe('LastInstallFlag', () => {
   beforeEach(() => {
-    FileSystem.emptyFolder(TEMP_DIR);
+    FileSystem.ensureEmptyFolder(TEMP_DIR);
   });
 
   afterEach(() => {
-    FileSystem.emptyFolder(TEMP_DIR);
+    FileSystem.ensureEmptyFolder(TEMP_DIR);
   });
 
   it('can create and remove a flag in an empty directory', () => {
diff --git a/apps/rush-lib/src/cli/actions/ChangeAction.ts b/apps/rush-lib/src/cli/actions/ChangeAction.ts
@@ -462,7 +462,7 @@ export class ChangeAction extends BaseRushAction {
    */
   private _writeFile(fileName: string, output: string): void {
     FileSystem.writeFile(fileName, output, {
-      ensureFolder: true
+      ensureFolderExists: true
     });
     console.log('Created file: ' + fileName);
   }
diff --git a/apps/rush-lib/src/logic/InstallManager.ts b/apps/rush-lib/src/logic/InstallManager.ts
@@ -850,17 +850,17 @@ export class InstallManager {
       // Before we start the network operation, record a failed state.  If the process exits for some reason,
       // this will record the error.  It will also update the timestamp to prevent other Rush instances
       // from attempting to update the file.
-      JsonFile.save('error', lastCheckFile, { ensureFolder: true });
+      JsonFile.save('error', lastCheckFile, { ensureFolderExists: true });
 
       // For this check we use the official registry, not the private registry
       return this._queryIfReleaseIsPublished('https://registry.npmjs.org:443')
         .then((publishedRelease: boolean) => {
           // Cache the result
-          JsonFile.save(publishedRelease, lastCheckFile, { ensureFolder: true });
+          JsonFile.save(publishedRelease, lastCheckFile, { ensureFolderExists: true });
           return publishedRelease;
         })
         .catch((error: Error) => {
-          JsonFile.save('error', lastCheckFile, { ensureFolder: true });
+          JsonFile.save('error', lastCheckFile, { ensureFolderExists: true });
           return Promise.reject(error);
         });
     });
diff --git a/apps/rush-lib/src/utilities/Utilities.ts b/apps/rush-lib/src/utilities/Utilities.ts
@@ -412,7 +412,7 @@ export class Utilities {
     if (FileSystem.exists(directory)) {
       console.log('Deleting old files from ' + directory);
     }
-    FileSystem.emptyFolder(directory);
+    FileSystem.ensureEmptyFolder(directory);
 
     const npmPackageJson: IPackageJson = {
       dependencies: {
diff --git a/core-build/gulp-core-build-karma/src/KarmaTask.ts b/core-build/gulp-core-build-karma/src/KarmaTask.ts
@@ -127,7 +127,7 @@ export class KarmaTask extends GulpTask<IKarmaTaskConfig> {
 
         const tempFolder: string = path.join(this.buildConfig.rootPath, this.buildConfig.tempFolder);
         FileSystem.writeFile(path.join(tempFolder, 'tests.js'), testsJsFileContents, {
-          ensureFolder: true
+          ensureFolderExists: true
         });
       }
 
diff --git a/core-build/gulp-core-build-serve/src/certificates.ts b/core-build/gulp-core-build-serve/src/certificates.ts
@@ -344,7 +344,7 @@ function _ensureCertificateInternal(parentTask: GulpTask<{}>): void {
 
   const tempCertificatePath: string = path.join(tempDirName, `${certificateName}.cer`);
   FileSystem.writeFile(tempCertificatePath, generatedCertificate.pemCertificate, {
-    ensureFolder: true
+    ensureFolderExists: true
   });
 
   if (_tryTrustCertificate(tempCertificatePath, parentTask)) {
diff --git a/core-build/gulp-core-build-typescript/src/TSLintTask.ts b/core-build/gulp-core-build-typescript/src/TSLintTask.ts
@@ -139,7 +139,7 @@ export class TSLintTask extends GulpTask<ITSLintTaskConfig> {
 
     const lintRulesFile: ITSLintRulesFile = self._loadLintConfiguration();
 
-    JsonFile.save(lintRulesFile, this._getTsLintFilepath(), { ensureFolder: true });
+    JsonFile.save(lintRulesFile, this._getTsLintFilepath(), { ensureFolderExists: true });
 
     const cached = require('gulp-cache'); // tslint:disable-line
 
@@ -152,7 +152,7 @@ export class TSLintTask extends GulpTask<ITSLintTaskConfig> {
           this.buildConfig.tempFolder,
           'tslint-tsconfig.json'
         );
-      JsonFile.save(tsconfigFileData, tsconfigFilePath, { ensureFolder: true });
+      JsonFile.save(tsconfigFileData, tsconfigFilePath, { ensureFolderExists: true });
 
       program = TSLint.Linter.createProgram(
         tsconfigFilePath,
diff --git a/libraries/node-core-library/src/FileDiffTest.ts b/libraries/node-core-library/src/FileDiffTest.ts
@@ -49,7 +49,7 @@ export class FileDiffTest {
     const diffTestPath: string = path.join(packageJsonFolderPath, 'temp', 'diff-tests', testModule);
     FileSystem.ensureFolder(diffTestPath);
 
-    FileSystem.emptyFolder(diffTestPath);
+    FileSystem.ensureEmptyFolder(diffTestPath);
 
     return diffTestPath;
   }
diff --git a/libraries/node-core-library/src/FileSystem.ts b/libraries/node-core-library/src/FileSystem.ts
@@ -75,7 +75,7 @@ export interface IWriteFileOptions {
    * If true, will ensure the folder is created before writing the file.
    * Defaults to `false`.
    */
-  ensureFolder?: boolean;
+  ensureFolderExists?: boolean;
 
   /**
    * If specified, will normalize line endings to the specified style of newline.
@@ -122,7 +122,7 @@ export interface IFileSystemMoveOptions {
    * If true, will ensure the folder is created before writing the file.
    * Defaults to `false`.
    */
-  ensureFolder?: boolean;
+  ensureFolderExists?: boolean;
 }
 
 /**
@@ -230,11 +230,11 @@ export class FileSystem {
   public static move(sourcePath: string, targetPath: string, options?: IFileSystemMoveOptions): void {
     options = {
       overwrite: false,
-      ensureFolder: false,
+      ensureFolderExists: false,
       ...options
     };
 
-    if (options.ensureFolder) {
+    if (options.ensureFolderExists) {
       FileSystem.ensureFolder(pathUtilities.basename(sourcePath));
     }
     fsx.moveSync(sourcePath, targetPath, { overwrite: options.overwrite });
@@ -268,7 +268,7 @@ export class FileSystem {
     };
 
     if (!FileSystem.exists(folderPath)) {
-      throw new Error(`Cannot read contents of folder, as it does not exist: "${folderPath}"`);
+      throw new Error(`Folder does not exist: "${folderPath}"`);
     }
 
     const fileNames: Array<string> = fsx.readdirSync(folderPath);
@@ -282,19 +282,24 @@ export class FileSystem {
 
   /**
    * Deletes a folder, including all of its contents.
-   * Behind the scenes is uses `fsx.removeSync()`
+   * Behind the scenes is uses `fsx.removeSync()`.
+   * @remarks
+   * Does not throw if the folderPath does not exist.
    * @param folderPath - The absolute or relative path to the folder which should be deleted.
    */
   public static deleteFolder(folderPath: string): void {
     fsx.removeSync(folderPath);
   }
 
   /**
-   * Deletes the content of a folder, but not the folder itself.
-   * Behind the scenes it uses `fsx.emptyDirSync()`
+   * Deletes the content of a folder, but not the folder itself. Also ensures the folder exists.
+   * Behind the scenes it uses `fsx.emptyDirSync()`.
+   * @remarks
+   * This is a workaround for a common race condition, where the virus scanner holds a lock on the folder
+   * for a brief period after it was deleted, causing EBUSY errors for any code that tries to recreate the folder.
    * @param folderPath - The absolute or relative path to the folder which should have its contents deleted.
    */
-  public static emptyFolder(folderPath: string): void {
+  public static ensureEmptyFolder(folderPath: string): void {
     fsx.emptyDirSync(folderPath);
   }
 
@@ -304,25 +309,27 @@ export class FileSystem {
 
   /**
    * Writes a text string to a file on disk, overwriting the file if it already exists.
-   * Behind the scenes it uses `fs.writeFileSync()`
+   * Behind the scenes it uses `fs.writeFileSync()`.
+   * @remarks
+   * Throws an error if the folder doesn't exist, unless ensureFolder=true.
    * @param filePath - The absolute or relative path of the file.
    * @param contents - The text that should be written to the file.
    * @param options - Optional settings that can change the behavior. Type: `IWriteFileOptions`
    */
-  public static writeFile(filePath: string, contents: string, options?: IWriteFileOptions): void {
+  public static writeFile(filePath: string, contents: string | Buffer, options?: IWriteFileOptions): void {
     options = {
-      ensureFolder: false,
+      ensureFolderExists: false,
       convertLineEndings: undefined,
       encoding: Encoding.Utf8,
       ...options
     };
 
-    if (options.ensureFolder) {
+    if (options.ensureFolderExists) {
       const folderPath: string = pathUtilities.dirname(filePath);
       FileSystem.ensureFolder(folderPath);
     }
 
-    contents = FileSystem._convertLineEndings(contents, options.convertLineEndings);
+    contents = FileSystem._convertLineEndings(contents.toString(), options.convertLineEndings);
 
     fsx.writeFileSync(filePath, contents, { encoding: options.encoding });
   }
diff --git a/libraries/node-core-library/src/JsonFile.ts b/libraries/node-core-library/src/JsonFile.ts
@@ -133,7 +133,7 @@ export class JsonFile {
     }
 
     FileSystem.writeFile(jsonFilename, buffer.toString(), {
-      ensureFolder: options.ensureFolder
+      ensureFolderExists: options.ensureFolder
     });
 
     // TEST CODE: Used to verify that onlyIfChanged isn't broken by a hidden transformation during saving.
diff --git a/libraries/node-core-library/src/test/LockFile.test.ts b/libraries/node-core-library/src/test/LockFile.test.ts
@@ -115,7 +115,7 @@ describe('LockFile', () => {
       it('can acquire and close a clean lockfile', () => {
         // ensure test folder is clean
         const testFolder: string = path.join(__dirname, '1');
-        FileSystem.emptyFolder(testFolder);
+        FileSystem.ensureEmptyFolder(testFolder);
 
         const resourceName: string = 'test';
         const pidLockFileName: string = LockFile.getLockFilePath(testFolder, resourceName);
@@ -141,7 +141,7 @@ describe('LockFile', () => {
       it('cannot acquire a lock if another valid lock exists', () => {
         // ensure test folder is clean
         const testFolder: string = path.join(__dirname, '2');
-        FileSystem.emptyFolder(testFolder);
+        FileSystem.ensureEmptyFolder(testFolder);
 
         const otherPid: number = 999999999;
         const otherPidStartTime: string = '2012-01-02 12:53:12';
diff --git a/stack/rush-stack/src/logic/BasicTasks.ts b/stack/rush-stack/src/logic/BasicTasks.ts
@@ -21,7 +21,7 @@ export class BasicTasks {
     for (const folderToClean of foldersToClean) {
       const fullPath: string = path.join(buildContext.projectFolder, folderToClean);
       console.log(`[clean]: Cleaning "${fullPath}"`);
-      FileSystem.emptyFolder(fullPath);
+      FileSystem.ensureEmptyFolder(fullPath);
     }
   }
 

Original file line number	Diff line number	Diff line change
`@@ -619,6 +619,6 @@ export class MarkdownDocumenter {`
`619`	`619`
`620`	`620`	`private _deleteOldOutputFiles(): void {`
`621`	`621`	`console.log('Deleting old output from ' + this._outputFolder);`
`622`		`- FileSystem.emptyFolder(this._outputFolder);`
	`622`	`+ FileSystem.ensureEmptyFolder(this._outputFolder);`
`623`	`623`	`}`
`624`	`624`	`}`
Original file line number	Diff line number	Diff line change
`@@ -420,7 +420,7 @@ export class YamlDocumenter {`
`420`	`420`
`421`	`421`	`FileSystem.writeFile(filePath, stringified, {`
`422`	`422`	`convertLineEndings: NewlineKind.CrLf,`
`423`		`- ensureFolder: true`
	`423`	`+ ensureFolderExists: true`
`424`	`424`	`});`
`425`	`425`
`426`	`426`	`if (schema) {`
`@@ -581,6 +581,6 @@ export class YamlDocumenter {`
`581`	`581`
`582`	`582`	`private _deleteOldOutputFiles(): void {`
`583`	`583`	`console.log('Deleting old output from ' + this._outputFolder);`
`584`		`- FileSystem.emptyFolder(this._outputFolder);`
	`584`	`+ FileSystem.ensureEmptyFolder(this._outputFolder);`
`585`	`585`	`}`
`586`	`586`	`}`
Original file line number	Diff line number	Diff line change
`@@ -117,7 +117,7 @@ export class DtsRollupGenerator {`
`117`	`117`
`118`	`118`	`FileSystem.writeFile(dtsFilename, indentedWriter.toString(), {`
`119`	`119`	`convertLineEndings: NewlineKind.CrLf,`
`120`		`- ensureFolder: true`
	`120`	`+ ensureFolderExists: true`
`121`	`121`	`});`
`122`	`122`	`}`
`123`	`123`
Original file line number	Diff line number	Diff line change
`@@ -66,7 +66,7 @@ export class ChangeFile {`
`66`	`66`	`public writeSync(): void {`
`67`	`67`	`const filePath: string = this.generatePath();`
`68`	`68`	`JsonFile.save(this._changeFileData, filePath, {`
`69`		`- ensureFolder: true`
	`69`	`+ ensureFolderExists: true`
`70`	`70`	`});`
`71`	`71`	`}`
`72`	`72`
Original file line number	Diff line number	Diff line change
`@@ -45,7 +45,7 @@ export class LastInstallFlag {`
`45`	`45`	`*/`
`46`	`46`	`public create(): void {`
`47`	`47`	`JsonFile.save(this._state, this._path, {`
`48`		`- ensureFolder: true`
	`48`	`+ ensureFolderExists: true`
`49`	`49`	`});`
`50`	`50`	`}`
`51`	`51`
Original file line number	Diff line number	Diff line change
`@@ -462,7 +462,7 @@ export class ChangeAction extends BaseRushAction {`
`462`	`462`	`*/`
`463`	`463`	`private _writeFile(fileName: string, output: string): void {`
`464`	`464`	`FileSystem.writeFile(fileName, output, {`
`465`		`- ensureFolder: true`
	`465`	`+ ensureFolderExists: true`
`466`	`466`	`});`
`467`	`467`	`console.log('Created file: ' + fileName);`
`468`	`468`	`}`