fix(source-maps): fix race condition between copying and purging source maps

Author: danbucholtz

src/util/source-maps.spec.ts

@@ -5,25 +5,87 @@ import * as helpers from './helpers';
 
 describe('source maps', () => {
   describe('purgeSourceMapsIfNeeded', () => {
-    it('should return a promise call unlink on all files with a .map extensin', () => {
-      // arrange
-      let env: any = {};
-      env[Constants.ENV_VAR_GENERATE_SOURCE_MAP] = null;
-      process.env = env;
-      const buildDir = '/some/fake/build/dir';
-      const context = { buildDir: buildDir };
-
-      spyOn(helpers, helpers.readDirAsync.name).and.returnValue(Promise.resolve(['test.js', 'test.js.map', 'test2.js', 'test2.js.map']));
-      const unlinkSpy = spyOn(helpers, helpers.unlinkAsync.name).and.returnValue(Promise.resolve());
-
-      // act
-      const resultPromise = sourceMaps.purgeSourceMapsIfNeeded(context);
-
-      // assert
-      return resultPromise.then(() => {
-        expect(unlinkSpy.calls.argsFor(0)[0]).toEqual(join(buildDir, 'test.js.map'));
-        expect(unlinkSpy.calls.argsFor(1)[0]).toEqual(join(buildDir, 'test2.js.map'));
+    it('should copy files first, then purge the files', async () => {
+      spyOn(helpers, helpers.getBooleanPropertyValue.name).and.callFake((argument: string) => {
+        if (argument === Constants.ENV_VAR_MOVE_SOURCE_MAPS) {
+          return true;
+        }
       });
+
+      spyOn(helpers, helpers.mkDirpAsync.name).and.returnValue(Promise.resolve());
+
+      const knownFileNames = ['0.js', '0.js.map', '1.js', '1.js.map', 'main.js', 'main.js.map', 'vendor.js', 'vendor.js.map', 'main.css', 'polyfills.js', 'sw-toolbox.js', 'main.css', 'main.css.map'];
+
+      spyOn(helpers, helpers.readDirAsync.name).and.returnValue(Promise.resolve(knownFileNames));
+
+      const context = {
+        sourcemapDir: join(process.cwd(), 'sourceMapDir'),
+        buildDir: join(process.cwd(), 'www', 'build')
+      };
+
+      const copyFileSpy = spyOn(helpers, helpers.copyFileAsync.name).and.returnValue(Promise.resolve());
+      const unlinkFileSpy = spyOn(helpers, helpers.unlinkAsync.name).and.returnValue(Promise.resolve());
+
+      const result = await sourceMaps.copySourcemaps(context, true);
+      expect(helpers.mkDirpAsync).toHaveBeenCalledTimes(1);
+      expect(helpers.mkDirpAsync).toHaveBeenCalledWith(context.sourcemapDir);
+
+      expect(helpers.readDirAsync).toHaveBeenCalledTimes(1);
+      expect(helpers.readDirAsync).toHaveBeenLastCalledWith(context.buildDir);
+
+      expect(helpers.copyFileAsync).toHaveBeenCalledTimes(3);
+      expect(copyFileSpy.calls.all()[0].args[0]).toEqual(join(context.buildDir, '0.js.map'));
+      expect(copyFileSpy.calls.all()[0].args[1]).toEqual(join(context.sourcemapDir, '0.js.map'));
+      expect(copyFileSpy.calls.all()[1].args[0]).toEqual(join(context.buildDir, '1.js.map'));
+      expect(copyFileSpy.calls.all()[1].args[1]).toEqual(join(context.sourcemapDir, '1.js.map'));
+      expect(copyFileSpy.calls.all()[2].args[0]).toEqual(join(context.buildDir, 'main.js.map'));
+      expect(copyFileSpy.calls.all()[2].args[1]).toEqual(join(context.sourcemapDir, 'main.js.map'));
+
+      expect(helpers.unlinkAsync).toHaveBeenCalledTimes(5);
+      expect(unlinkFileSpy.calls.all()[0].args[0]).toEqual(join(context.buildDir, '0.js.map'));
+      expect(unlinkFileSpy.calls.all()[1].args[0]).toEqual(join(context.buildDir, '1.js.map'));
+      expect(unlinkFileSpy.calls.all()[2].args[0]).toEqual(join(context.buildDir, 'main.js.map'));
+      expect(unlinkFileSpy.calls.all()[3].args[0]).toEqual(join(context.buildDir, 'vendor.js.map'));
+      expect(unlinkFileSpy.calls.all()[4].args[0]).toEqual(join(context.buildDir, 'main.css.map'));
+    });
+
+    it('should copy the files but not purge them after', async () => {
+      spyOn(helpers, helpers.getBooleanPropertyValue.name).and.callFake((argument: string) => {
+        if (argument === Constants.ENV_VAR_MOVE_SOURCE_MAPS) {
+          return true;
+        }
+      });
+
+      spyOn(helpers, helpers.mkDirpAsync.name).and.returnValue(Promise.resolve());
+
+      const knownFileNames = ['0.js', '0.js.map', '1.js', '1.js.map', 'main.js', 'main.js.map', 'vendor.js', 'vendor.js.map', 'main.css', 'polyfills.js', 'sw-toolbox.js', 'main.css', 'main.css.map'];
+
+      spyOn(helpers, helpers.readDirAsync.name).and.returnValue(Promise.resolve(knownFileNames));
+
+      const context = {
+        sourcemapDir: join(process.cwd(), 'sourceMapDir'),
+        buildDir: join(process.cwd(), 'www', 'build')
+      };
+
+      const copyFileSpy = spyOn(helpers, helpers.copyFileAsync.name).and.returnValue(Promise.resolve());
+      const unlinkFileSpy = spyOn(helpers, helpers.unlinkAsync.name).and.returnValue(Promise.resolve());
+
+      const result = await sourceMaps.copySourcemaps(context, false);
+      expect(helpers.mkDirpAsync).toHaveBeenCalledTimes(1);
+      expect(helpers.mkDirpAsync).toHaveBeenCalledWith(context.sourcemapDir);
+
+      expect(helpers.readDirAsync).toHaveBeenCalledTimes(1);
+      expect(helpers.readDirAsync).toHaveBeenLastCalledWith(context.buildDir);
+
+      expect(helpers.copyFileAsync).toHaveBeenCalledTimes(3);
+      expect(copyFileSpy.calls.all()[0].args[0]).toEqual(join(context.buildDir, '0.js.map'));
+      expect(copyFileSpy.calls.all()[0].args[1]).toEqual(join(context.sourcemapDir, '0.js.map'));
+      expect(copyFileSpy.calls.all()[1].args[0]).toEqual(join(context.buildDir, '1.js.map'));
+      expect(copyFileSpy.calls.all()[1].args[1]).toEqual(join(context.sourcemapDir, '1.js.map'));
+      expect(copyFileSpy.calls.all()[2].args[0]).toEqual(join(context.buildDir, 'main.js.map'));
+      expect(copyFileSpy.calls.all()[2].args[1]).toEqual(join(context.sourcemapDir, 'main.js.map'));
+
+      expect(helpers.unlinkAsync).toHaveBeenCalledTimes(0);
     });
   });
 });

src/util/source-maps.ts

@@ -1,38 +1,50 @@
 import { join, relative, basename } from 'path';
-import { mkdirpSync } from 'fs-extra';
+import { ensureDir, mkdirpSync } from 'fs-extra';
 import * as Constants from './constants';
-import { copyFileAsync, getBooleanPropertyValue, readDirAsync, unlinkAsync } from './helpers';
+import { copyFileAsync, getBooleanPropertyValue, mkDirpAsync, readDirAsync, unlinkAsync } from './helpers';
 import { BuildContext } from './interfaces';
 
-function copySourcemaps(context: BuildContext, shouldPurge: boolean) {
-  return readDirAsync(context.buildDir).then((fileNames) => {
-    const sourceMaps = fileNames.filter(fileName => fileName.endsWith('.map'));
-    const fullPaths = sourceMaps.map(sourceMap => join(context.buildDir, sourceMap));
-    const promises: Promise<void>[] = [];
-    const copyBeforePurge = getBooleanPropertyValue(Constants.ENV_VAR_MOVE_SOURCE_MAPS);
-    for (const fullPath of fullPaths) {
-      if (copyBeforePurge) {
-        mkdirpSync(context.sourcemapDir);
-        const relativeTo = relative(fullPath, context.sourcemapDir);
-        const fileName = basename(fullPath);
-        if (fileName.indexOf('vendor.js') < 0) {
-          promises.push(copyFileAsync(fullPath, join(context.sourcemapDir, fileName)));
-        }
-      }
-
-      if (shouldPurge) {
-        promises.push(unlinkAsync(fullPath));
-      }
+export async function copySourcemaps(context: BuildContext, shouldPurge: boolean) {
+  const copyBeforePurge = getBooleanPropertyValue(Constants.ENV_VAR_MOVE_SOURCE_MAPS);
+  if (copyBeforePurge) {
+    await mkDirpAsync(context.sourcemapDir);
+  }
+
+  const fileNames = await readDirAsync(context.buildDir);
+
+  // only include js source maps
+  const sourceMaps = fileNames.filter(fileName => fileName.endsWith('.map'));
+
+  const toCopy = sourceMaps.filter(fileName => fileName.indexOf('vendor.js') < 0 && fileName.endsWith('.js.map'));
+  const toCopyFullPaths = toCopy.map(fileName => join(context.buildDir, fileName));
+
+  const toPurge = sourceMaps.map(sourceMap => join(context.buildDir, sourceMap));
+
+  const copyFilePromises: Promise<any>[] = [];
+  if (copyBeforePurge) {
+    for (const fullPath of toCopyFullPaths) {
+      const fileName = basename(fullPath);
+      copyFilePromises.push(copyFileAsync(fullPath, join(context.sourcemapDir, fileName)));
     }
-    return Promise.all(promises);
-  });
+  }
+
+  await Promise.all(copyFilePromises);
+
+  // okay cool, all of the files have been copied over, so go ahead and blow them all away
+  const purgeFilePromises: Promise<any>[] = [];
+  if (shouldPurge) {
+    for (const fullPath of toPurge) {
+      purgeFilePromises.push(unlinkAsync(fullPath));
+    }
+  }
+
+  return await Promise.all(purgeFilePromises);
 }
 
 export function purgeSourceMapsIfNeeded(context: BuildContext): Promise<any> {
   if (getBooleanPropertyValue(Constants.ENV_VAR_GENERATE_SOURCE_MAP)) {
     // keep the source maps and just return
     return copySourcemaps(context, false);
   }
-
   return copySourcemaps(context, true);
 }