Inline includes (#819)

* Inline includes

Rather than making the content of the included file the _contents_ of
the `<include />` node, we replace the `<include />` node with the
children of the included file.

* Use asyncTreeFlatMap instead of asyncTreeMap

FlatMap is more powerful than map. More importantly, I only want to maintain one tree map function if I can help it.

experiments/modern/src/App.js

@@ -55,7 +55,7 @@ async function getSkin() {
   // const system = new System();
 
   const images = {};
-  await Utils.asyncTreeMap(skinXml, async node => {
+  await Utils.asyncTreeFlatMap(skinXml, async node => {
     // TODO: This is probalby only valid if in an `<elements>` node
     switch (node.name) {
       case "bitmap": {

experiments/modern/src/utils.js

@@ -23,6 +23,8 @@ export async function readUint8array(zip, filepath) {
   return file.async("uint8array");
 }
 
+// I any of the values in `arr` are themselves arrays, interpolate the nested
+// array into the top level array.
 function flatten(arr) {
   const newArr = [];
   arr.forEach(item => {
@@ -35,49 +37,53 @@ function flatten(arr) {
   return newArr;
 }
 
-// Note: Bad things will happen if the root node you pass returns an array. In
-// our use case, the root node never returns an array.
-// Note: Because children are processed in parallel, the order in which nodes
-// are processed can be a bit confusing. The only guarantee we make is that
-// children will be processed before their parent.
-export async function asyncDepthFirstFlatMap(node, mapper) {
+// Map an async function over an array. If the value returned from the mapper is
+// an array, it recursively maps the function over that array's values, and then
+// interpoates the resulting flat array into the top level array of results.
+export async function asyncFlatMap(arr, mapper) {
+  const mapped = await Promise.all(arr.map(mapper));
+  const childPromises = mapped.map(async item => {
+    if (Array.isArray(item)) {
+      return await asyncFlatMap(item, mapper);
+    } else {
+      return item;
+    }
+  });
+  return flatten(await Promise.all(childPromises));
+}
+
+// Apply a mapper function to all nodes in a tree using `asyncFlatMap`. This
+// allows mapper to conditionally return either a single node, or an array of
+// nodes.
+//
+// The tree should have the form of objects nodes, each of which may
+// have a property `children` which is an array of nodes.
+//
+// Note: The root node will not be transformed by the mapper, since the mapper
+// could potentially return multiple nodes.
+export async function asyncTreeFlatMap(node, mapper) {
   const { children } = node;
   if (children == null) {
-    return mapper(node);
+    return node;
   }
 
-  const promisedChildren = children.map(child =>
-    asyncDepthFirstFlatMap(child, mapper)
+  const mappedChildren = await asyncFlatMap(children, mapper);
+
+  const recursedChildren = await Promise.all(
+    mappedChildren.map(child => asyncTreeFlatMap(child, mapper))
   );
-  const mappedChildren = await Promise.all(promisedChildren);
-  const flatChildren = flatten(mappedChildren);
 
-  return mapper({ ...node, children: flatChildren });
-}
-
-// Transform an tree structure by mapping over each node breadth first
-// Parents are mapped before their children
-// Children are mapped in parallel
-export async function asyncTreeMap(node, mapper) {
-  const mapped = await mapper(node);
-  if (mapped.children == null) {
-    return mapped;
-  }
-  const promises = mapped.children.map(child => {
-    return asyncTreeMap(child, mapper);
-  });
-  const children = await Promise.all(promises);
-
-  return { ...mapped, children };
+  return { ...node, children: recursedChildren };
 }
 
+// Given an XML file and a zip which it came from, replace all `<inline />` elements
+// with the contents of the file to be included.
 export async function inlineIncludes(xml, zip) {
-  return asyncTreeMap(xml, async node => {
+  return asyncTreeFlatMap(xml, async node => {
     if (node.name !== "include") {
       return node;
     }
     // TODO: Normalize file names so that they hit the same cache
-    // TODO: Ensure this node does not already have children for some reason
     const includedFile = await readXml(zip, node.attributes.file);
     if (includedFile == null) {
       console.warn(
@@ -87,6 +93,6 @@ export async function inlineIncludes(xml, zip) {
       );
       return node;
     }
-    return { ...node, children: includedFile.children };
+    return includedFile.children;
   });
 }

experiments/modern/src/utils.test.js

@@ -25,40 +25,58 @@ describe("readXml", () => {
   });
 });
 
-describe("asyncTreeMap", () => {
-  it("runs parents before children", async () => {
-    const callNodeNames = new Set();
-    const mapper = node => {
-      callNodeNames.add(node.name);
-      if (node.name === "root.2") {
-        const children = [{ name: "root.2.1" }];
-        return { ...node, children };
+describe("inlineIncludes", () => {
+  test("asyncTreeFlatMap", async () => {
+    const playerElements = {
+      name: "player-elements",
+      children: [{ name: "player-elements-child" }],
+    };
+    const playerNormal = {
+      name: "player-normal",
+      children: [{ name: "player-normal-child" }],
+    };
+    const player = {
+      name: "player",
+      children: [
+        {
+          name: "player-elements-include",
+          include: playerElements,
+        },
+        {
+          name: "main-container",
+          children: [
+            {
+              name: "player-normal-include",
+              include: playerNormal,
+            },
+          ],
+        },
+      ],
+    };
+
+    const xml = {
+      name: "root",
+      children: [{ name: "meta" }, { name: "include player", include: player }],
+    };
+
+    function resolveInclude(node) {
+      if (node.include) {
+        return node.include.children;
       }
       return node;
-    };
-
-    const structure = {
-      name: "root",
-      children: [{ name: "root.1" }, { name: "root.2" }, { name: "root.3" }],
-    };
-
-    const mappedStructure = await Utils.asyncTreeMap(structure, mapper);
-    expect(callNodeNames).toEqual(
-      new Set(["root", "root.1", "root.2", "root.2.1", "root.3"])
-    );
-    expect(mappedStructure).toEqual({
+    }
+    const resolved = await Utils.asyncTreeFlatMap(xml, resolveInclude);
+    expect(resolved).toEqual({
       name: "root",
       children: [
-        { name: "root.1" },
-        { name: "root.2", children: [{ name: "root.2.1" }] },
-        { name: "root.3" },
+        { name: "meta" },
+        { name: "player-elements-child" },
+        { name: "main-container", children: [{ name: "player-normal-child" }] },
       ],
     });
   });
-});
 
-describe("inlineIncludes", () => {
-  it("inlines the contents of included files as children of the include node", async () => {
+  test("inlines the contents of included files as children of the include node", async () => {
     const zip = await getSkinZip();
     const originalFile = zip.file;
     zip.file = jest.fn(path => originalFile.call(zip, path));
@@ -85,16 +103,27 @@ describe("inlineIncludes", () => {
   });
 });
 
-describe("asyncDepthFirstFlatMap", () => {
+describe("asyncFlatMap", () => {
+  test("recurses", async () => {
+    const start = ["parent", ["child", ["grandchild"], "sibling"], "partner"];
+    expect(await Utils.asyncFlatMap(start, v => Promise.resolve(v))).toEqual([
+      "parent",
+      "child",
+      "grandchild",
+      "sibling",
+      "partner",
+    ]);
+  });
+});
+
+describe("asyncTreeFlatMap", () => {
   test("encounters children first", async () => {
-    const encounterd = [];
-    const mapper = async node => {
-      encounterd.push(node.name);
+    const mapper = jest.fn(async node => {
       if (node.replaceWithChildren) {
         return node.children;
       }
       return { ...node, name: node.name.toLowerCase() };
-    };
+    });
 
     const start = {
       name: "A",
@@ -111,10 +140,12 @@ describe("asyncDepthFirstFlatMap", () => {
         { name: "D" },
       ],
     };
-    expect(await Utils.asyncDepthFirstFlatMap(start, mapper)).toEqual({
-      name: "a",
+    expect(await Utils.asyncTreeFlatMap(start, mapper)).toEqual({
+      name: "A",
       children: [{ name: "b" }, { name: "e" }, { name: "g" }, { name: "d" }],
     });
-    expect(encounterd).toEqual(["B", "E", "G", "D", "F", "C", "A"]);
+
+    const callOrder = mapper.mock.calls.map(args => args[0].name);
+    expect(callOrder).toEqual(["B", "C", "D", "E", "F", "G"]);
   });
 });