Metro-Services Fixes (#393)

* Add a tolerance when finding a matching scale factor for android. Move "throw" statement into scale search routine, so that it always returns a real value (or throws). Add tests.

* Remove dependency on mkdirp, and use nodejs "fs" instead.

* Change files

* PR feedback

change/@rnx-kit-metro-service-03514734-4444-485e-93ea-6393e433c081.json

@@ -0,0 +1,7 @@
+{
+  "type": "patch",
+  "comment": "Remove dependency on mkdirp, and use nodejs \"fs\" instead. Add a tolerance when finding a matching scale factor for android. Move \"throw\" statement into scale search routine, so that it always returns a real value (or throws). Add tests.",
+  "packageName": "@rnx-kit/metro-service",
+  "email": "afoxman@microsoft.com",
+  "dependentChangeType": "patch"
+}

packages/metro-service/package.json

@@ -18,9 +18,11 @@
     "format": "rnx-kit-scripts format",
     "test": "rnx-kit-scripts test"
   },
+  "engines": {
+    "node": ">= 10.12"
+  },
   "dependencies": {
-    "chalk": "^4.1.0",
-    "mkdirp": "^0.5.3"
+    "chalk": "^4.1.0"
   },
   "peerDependencies": {
     "metro": ">=0.66.1",
@@ -36,7 +38,6 @@
     "@types/metro-babel-transformer": "*",
     "@types/metro-config": "*",
     "@types/metro-resolver": "*",
-    "@types/mkdirp": "^0.5.2",
     "@types/node": "^14.15.0",
     "metro": "^0.66.1",
     "metro-config": "^0.66.1",
@@ -52,5 +53,11 @@
   },
   "eslintConfig": {
     "extends": "@rnx-kit/eslint-config"
+  },
+  "jest": {
+    "roots": [
+      "test"
+    ],
+    "testRegex": "/test/.*\\.test\\.ts$"
   }
 }

packages/metro-service/src/asset/android.ts

@@ -1,27 +1,38 @@
 import path from "path";
 import type { PackagerAsset } from "./types";
 
-/**
- * FIXME: using number to represent discrete scale numbers is fragile in essence because of
- * floating point numbers imprecision.
- */
-function getAndroidAssetSuffix(scale: number): string {
-  switch (scale) {
-    case 0.75:
-      return "ldpi";
-    case 1:
-      return "mdpi";
-    case 1.5:
-      return "hdpi";
-    case 2:
-      return "xhdpi";
-    case 3:
-      return "xxhdpi";
-    case 4:
-      return "xxxhdpi";
-    default:
-      return "";
+function isApproximatelyEqual(
+  f1: number,
+  f2: number,
+  tolerance: number
+): boolean {
+  return Math.abs(f1 - f2) < tolerance;
+}
+
+export function getAndroidAssetSuffix(
+  asset: PackagerAsset,
+  scale: number
+): string {
+  const tolerance = 0.01;
+  if (isApproximatelyEqual(scale, 0.75, tolerance)) {
+    return "ldpi";
+  } else if (isApproximatelyEqual(scale, 1, tolerance)) {
+    return "mdpi";
+  } else if (isApproximatelyEqual(scale, 1.5, tolerance)) {
+    return "hdpi";
+  } else if (isApproximatelyEqual(scale, 2, tolerance)) {
+    return "xhdpi";
+  } else if (isApproximatelyEqual(scale, 3, tolerance)) {
+    return "xxhdpi";
+  } else if (isApproximatelyEqual(scale, 4, tolerance)) {
+    return "xxxhdpi";
   }
+
+  throw new Error(
+    `Don't know which android drawable suffix to use for asset: ${JSON.stringify(
+      asset
+    )}`
+  );
 }
 
 // See https://developer.android.com/guide/topics/resources/drawable-resource.html
@@ -41,16 +52,7 @@ function getAndroidResourceFolderName(
   if (!drawableFileTypes.has(asset.type)) {
     return "raw";
   }
-  const suffix = getAndroidAssetSuffix(scale);
-  if (!suffix) {
-    throw new Error(
-      `Don't know which android drawable suffix to use for asset: ${JSON.stringify(
-        asset
-      )}`
-    );
-  }
-  const androidFolder = `drawable-${suffix}`;
-  return androidFolder;
+  return `drawable-${getAndroidAssetSuffix(asset, scale)}`;
 }
 
 function getBasePath(asset: PackagerAsset): string {

packages/metro-service/src/asset/write.ts

@@ -1,55 +1,28 @@
 import fs from "fs";
 import path from "path";
-import mkdirp from "mkdirp";
 
 import type { AssetData } from "metro";
 
 import { getAssetDestPathAndroid } from "./android";
 import { getAssetDestPathIOS } from "./ios";
 import { filterPlatformAssetScales } from "./filter";
 
-function copy(
-  src: string,
-  dest: string,
-  callback: (error: NodeJS.ErrnoException) => void
-): void {
+function copy(src: string, dest: string): void {
   const destDir = path.dirname(dest);
-  mkdirp(destDir, (err?: NodeJS.ErrnoException) => {
-    if (err) {
-      callback(err);
-      return;
-    }
-    fs.createReadStream(src)
-      .pipe(fs.createWriteStream(dest))
-      .on("finish", callback);
-  });
+  fs.mkdirSync(destDir, { recursive: true });
+  fs.copyFileSync(src, dest);
 }
 
-async function copyAll(filesToCopy: Record<string, string>): Promise<void> {
+function copyAll(filesToCopy: Record<string, string>): void {
   const queue = Object.keys(filesToCopy);
   if (queue.length === 0) {
-    return Promise.resolve();
+    return;
   }
 
   console.info(`Copying ${queue.length} asset files`);
-  return new Promise((resolve, reject) => {
-    const copyNext = (error?: NodeJS.ErrnoException) => {
-      if (error) {
-        reject(error);
-        return;
-      }
-      if (queue.length === 0) {
-        console.info("Done copying assets");
-        resolve();
-      } else {
-        // queue.length === 0 is checked in previous branch, so this is string
-        const src = queue.shift() as string;
-        const dest = filesToCopy[src];
-        copy(src, dest, copyNext);
-      }
-    };
-    copyNext();
-  });
+  for (const src of queue) {
+    copy(src, filesToCopy[src]);
+  }
 }
 
 export async function saveAssets(
@@ -80,5 +53,6 @@ export async function saveAssets(
     });
   });
 
-  return copyAll(filesToCopy);
+  copyAll(filesToCopy);
+  return Promise.resolve();
 }

packages/metro-service/test/asset/__snapshots__/android.test.ts.snap

@@ -0,0 +1,5 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`MetroService getAndroidAssetSuffix throw for invalid scale 0.70 1`] = `"Don't know which android drawable suffix to use for asset: {\\"httpServerLocation\\":\\"https://my-server.com/assets\\",\\"name\\":\\"logo\\",\\"type\\":\\"jpg\\"}"`;
+
+exports[`MetroService getAndroidAssetSuffix throw for invalid scale 1.23 1`] = `"Don't know which android drawable suffix to use for asset: {\\"httpServerLocation\\":\\"https://my-server.com/assets\\",\\"name\\":\\"logo\\",\\"type\\":\\"jpg\\"}"`;

packages/metro-service/test/asset/android.test.ts

@@ -0,0 +1,54 @@
+import type { PackagerAsset } from "../../src/asset/types";
+import { getAndroidAssetSuffix } from "../../src/asset/android";
+
+describe("MetroService", () => {
+  const asset: PackagerAsset = {
+    httpServerLocation: "https://my-server.com/assets",
+    name: "logo",
+    type: "jpg",
+  };
+
+  test("getAndroidAssetSuffix returns ldpi for scale 0.75", () => {
+    expect(getAndroidAssetSuffix(asset, 0.75)).toEqual("ldpi");
+  });
+
+  test("getAndroidAssetSuffix returns mdpi for scale 1", () => {
+    expect(getAndroidAssetSuffix(asset, 1)).toEqual("mdpi");
+  });
+
+  test("getAndroidAssetSuffix returns hdpi for scale 1.5", () => {
+    expect(getAndroidAssetSuffix(asset, 1.5)).toEqual("hdpi");
+  });
+
+  test("getAndroidAssetSuffix returns xhdpi for scale 2", () => {
+    expect(getAndroidAssetSuffix(asset, 2)).toEqual("xhdpi");
+  });
+
+  test("getAndroidAssetSuffix returns xxhdpi for scale 3", () => {
+    expect(getAndroidAssetSuffix(asset, 3)).toEqual("xxhdpi");
+  });
+
+  test("getAndroidAssetSuffix returns xxxhdpi for scale 4", () => {
+    expect(getAndroidAssetSuffix(asset, 4)).toEqual("xxxhdpi");
+  });
+
+  test("getAndroidAssetSuffix returns ldpi for scale 0.741", () => {
+    expect(getAndroidAssetSuffix(asset, 0.741)).toEqual("ldpi");
+  });
+
+  test("getAndroidAssetSuffix returns ldpi for scale 0.759", () => {
+    expect(getAndroidAssetSuffix(asset, 0.759)).toEqual("ldpi");
+  });
+
+  test("getAndroidAssetSuffix throw for invalid scale 0.70", () => {
+    expect(() =>
+      getAndroidAssetSuffix(asset, 0.7)
+    ).toThrowErrorMatchingSnapshot();
+  });
+
+  test("getAndroidAssetSuffix throw for invalid scale 1.23", () => {
+    expect(() =>
+      getAndroidAssetSuffix(asset, 1.23)
+    ).toThrowErrorMatchingSnapshot();
+  });
+});

yarn.lock

@@ -1911,13 +1911,6 @@
   resolved "https://registry.yarnpkg.com/@types/minimatch/-/minimatch-3.0.4.tgz#f0ec25dbf2f0e4b18647313ac031134ca5b24b21"
   integrity sha512-1z8k4wzFnNjVK/tlxvrWuK5WMt6mydWWP7+zvH5eFep4oj+UkrfiJTRtjCeBXNpwaA/FYqqtb4/QS4ianFpIRA==
 
-"@types/mkdirp@^0.5.2":
-  version "0.5.2"
-  resolved "https://registry.yarnpkg.com/@types/mkdirp/-/mkdirp-0.5.2.tgz#503aacfe5cc2703d5484326b1b27efa67a339c1f"
-  integrity sha512-U5icWpv7YnZYGsN4/cmh3WD2onMY0aJIiTE6+51TwJCttdHvtCYmkBNOobHlXwrJRL0nkH9jH4kD+1FAdMN4Tg==
-  dependencies:
-    "@types/node" "*"
-
 "@types/node-fetch@^2.5.0":
   version "2.5.9"
   resolved "https://registry.yarnpkg.com/@types/node-fetch/-/node-fetch-2.5.9.tgz#c04a12115aa436f189e39579272b305e477621b4"
@@ -7463,7 +7456,7 @@ mkdirp@1.x, mkdirp@^1.0.3:
   resolved "https://registry.yarnpkg.com/mkdirp/-/mkdirp-1.0.4.tgz#3eb5ed62622756d79a5f0e2a221dfebad75c2f7e"
   integrity sha512-vVqVZQyf3WLx2Shd0qJ9xuvqgAyKPLAiqITEtqW0oIUjzo3PePDd6fW9iFz30ef7Ysp/oiWqbhszeGWW2T6Gzw==
 
-mkdirp@^0.5.1, mkdirp@^0.5.3:
+mkdirp@^0.5.1:
   version "0.5.5"
   resolved "https://registry.yarnpkg.com/mkdirp/-/mkdirp-0.5.5.tgz#d91cefd62d1436ca0f41620e251288d420099def"
   integrity sha512-NKmAlESf6jMGym1++R0Ra7wvhV+wFW63FaSOFPwRahvea0gMUcGUhVeAg/0BC0wiv9ih5NYPB1Wn1UEI1/L+xQ==