feat: send onDone notifications during init (#48)

This sends `onBegin` and `onDone` during init for files which have already been completed in a
previous session

Author: fivecar

.husky/pre-commit

@@ -2,3 +2,4 @@
 . "$(dirname "$0")/_/husky.sh"
 
 npx lint-staged
+npm test

README.md

@@ -65,6 +65,8 @@ For full documentation, see the javaDoc style comments in the package which auto
 
 Gets everything started (e.g. reconstitutes state from storage and reconciles it with downloads that might have completed in the background, subscribes to events, etc). You must call this first.
 
+During initialization, `onBegin` and `onDone` handlers will be called for any files that were already successfully downloaded in previous app sessions, ensuring your app knows about all available files.
+
 You can pass any of the following options, or nothing at all:
 
 | Option | Type | Default | Description |
@@ -81,9 +83,9 @@ Here are the optional notification handlers you can pass to be informed of downl
 
 | Handler | Description |
 |---|---|
-|`onBegin?: (url: string, totalBytes: number) => void` | Called when the download has begun and the total number of bytes expected is known.|
+|`onBegin?: (url: string, totalBytes: number) => void` | Called when the download has begun and the total number of bytes expected is known. Also called during `init()` for files that were already downloaded, just before `onDone` is called.|
 |`onProgress?: (url: string, fractionWritten: number, bytesWritten: number, totalBytes: number) => void` | Called at most every 1.5 seconds for any file while it's downloading. `fractionWritten` is between 0.0 and 1.0|
-|`onDone?: (url: string, localPath: string) => void`| Called when the download has completed successfully. `localPath` will be a file path.|
+|`onDone?: (url: string, localPath: string) => void`| Called when the download has completed successfully. `localPath` will be a file path. This is also called during `init()` for any files that were already downloaded in previous app sessions, giving you a complete picture of all available files.|
 |`onWillRemove?: (url: string) => Promise<void>`| Called before any url is removed from the queue. This is async because `removeUrl` (and also `setQueue`, when it needs to remove some urls) will block until you return from this, giving you the opportunity remove any dependencies on any downloaded local file before it's deleted.|
 |`onError?: (url: string, error: any) => void`| Called when there's been an issue downloading the file. Note that this is mostly for you to communicate something to the user, or to do other housekeeping; DownloadQueue will automatically re-attempt the download every minute (while you're online) until it succeeds.|
 

jest.config.js

@@ -4,4 +4,18 @@ module.exports = {
   testEnvironment: "node",
   transformIgnorePatterns: ["/node_modules/(?!(@?react-native))/"],
   setupFiles: ["./jest.setup.js"],
+  collectCoverageFrom: [
+    "src/**/*.{js,jsx,ts,tsx}",
+    "!src/**/*.d.ts",
+    "!lib/**/*",
+    "!**/node_modules/**",
+  ],
+  coverageThreshold: {
+    global: {
+      branches: 100,
+      functions: 100,
+      lines: 100,
+      statements: 100,
+    },
+  },
 };

src/index.ts

@@ -282,11 +282,27 @@ export default class DownloadQueue {
     // Now start downloads for specs that haven't finished
     await Promise.all(
       this.specs.map(async spec => {
-        if (existingTasks.some(task => task.id === spec.id)) {
+        if (
+          existingTasks.some(task => task.id === spec.id) ||
+          spec.createTime <= 0
+        ) {
           return;
         }
         await this.reconcileFinishStateWithFile(spec);
-        if (!spec.finished && spec.createTime > 0) {
+        if (spec.finished) {
+          // Notify handlers about already-finished specs without tasks
+          try {
+            const fileSpec = await RNFS.stat(spec.path);
+
+            this.handlers?.onBegin?.(spec.url, fileSpec.size);
+            this.handlers?.onDone?.(spec.url, spec.path);
+          } catch {
+            // File doesn't exist, treat as not finished
+            spec.finished = false;
+            await this.kvfs.write(this.keyFromId(spec.id), spec);
+            this.start(spec);
+          }
+        } else {
           this.start(spec);
         }
       })
@@ -927,9 +943,19 @@ export default class DownloadQueue {
         task.stop();
       }
 
+      if (!spec) {
+        return;
+      }
+
       // Given logic in the bigger "if" above, only unfinished lazy-deletes
       // should pass this.
-      if (spec && !spec.finished) {
+      if (spec.finished) {
+        if (spec.createTime > 0) {
+          // Notify handlers about already-finished specs
+          this.handlers?.onBegin?.(spec.url, task.bytesTotal);
+          this.handlers?.onDone?.(spec.url, spec.path);
+        }
+      } else {
         try {
           // There might be a partially downloaded file on disk. We need to
           // get rid of it in case a lazy-delete spec is revived, at which

test/index.spec.ts

@@ -494,6 +494,74 @@ describe("DownloadQueue", () => {
       expect(handlers.onDone).not.toHaveBeenCalled();
     });
 
+    it("should call onDone for already-finished specs with existing tasks", async () => {
+      const queue = new DownloadQueue();
+      const handlers: DownloadQueueHandlers = {
+        onBegin: jest.fn(),
+        onDone: jest.fn(),
+      };
+
+      task.state = "STOPPED";
+      task.bytesTotal = 8675309;
+      (checkForExistingDownloads as jest.Mock).mockReturnValue([task]);
+      (exists as jest.Mock).mockReturnValue(true);
+
+      await kvfs.write("/mydomain/foo", {
+        id: "foo",
+        url: "http://foo.com/a.mp3",
+        path: `${RNFS.DocumentDirectoryPath}/DownloadQueue/mydomain/foo`,
+        createTime: Date.now() - 1000,
+        finished: true,
+      });
+      await queue.init({ domain: "mydomain", handlers });
+
+      // Should not restart download for finished specs
+      expect(download).not.toHaveBeenCalled();
+      expect(task.resume).not.toHaveBeenCalled();
+
+      // Should call handlers for the finished spec
+      expect(handlers.onBegin).toHaveBeenCalledWith(
+        "http://foo.com/a.mp3",
+        task.bytesTotal
+      );
+      expect(handlers.onDone).toHaveBeenCalledWith(
+        "http://foo.com/a.mp3",
+        `${RNFS.DocumentDirectoryPath}/DownloadQueue/mydomain/foo`
+      );
+    });
+
+    it("should not call onDone for lazy-deleted finished specs with existing tasks", async () => {
+      const queue = new DownloadQueue();
+      const handlers: DownloadQueueHandlers = {
+        onBegin: jest.fn(),
+        onDone: jest.fn(),
+      };
+
+      task.state = "PAUSED"; // Use PAUSED so isTaskDownloading returns true
+      task.bytesTotal = 8675309;
+      (checkForExistingDownloads as jest.Mock).mockReturnValue([task]);
+      (exists as jest.Mock).mockReturnValue(true);
+
+      await kvfs.write("/mydomain/foo", {
+        id: "foo",
+        url: "http://foo.com/a.mp3",
+        path: `${RNFS.DocumentDirectoryPath}/DownloadQueue/mydomain/foo`,
+        createTime: -(Date.now() + 1000), // Lazy delete
+        finished: true,
+      });
+      await queue.init({ domain: "mydomain", handlers });
+
+      // Should stop the task since it's downloading/paused
+      expect(task.stop).toHaveBeenCalled();
+
+      // Should not call handlers for lazy-deleted specs
+      expect(handlers.onBegin).not.toHaveBeenCalled();
+      expect(handlers.onDone).not.toHaveBeenCalled();
+
+      // Should not restart download
+      expect(download).not.toHaveBeenCalled();
+    });
+
     it("restarts failed specs from previous launches", async () => {
       const queue = new DownloadQueue();
       const handlers: DownloadQueueHandlers = {
@@ -644,6 +712,70 @@ describe("DownloadQueue", () => {
       expect(download).not.toHaveBeenCalled();
     });
 
+    it("should call onDone for already-finished specs without tasks", async () => {
+      const queue = new DownloadQueue();
+      const handlers: DownloadQueueHandlers = {
+        onBegin: jest.fn(),
+        onDone: jest.fn(),
+      };
+
+      (exists as jest.Mock).mockImplementation(() => true);
+      (stat as jest.Mock).mockReturnValue({ size: 8675309 });
+
+      await kvfs.write("/mydomain/foo", {
+        id: "foo",
+        url: "http://foo.com/a.mp3",
+        path: `${RNFS.DocumentDirectoryPath}/DownloadQueue/mydomain/foo`,
+        createTime: Date.now() - 1000,
+        finished: true,
+      });
+
+      await queue.init({ domain: "mydomain", handlers });
+
+      // Should not start a new download for finished specs
+      expect(download).not.toHaveBeenCalled();
+
+      // Should call handlers for the finished spec
+      expect(handlers.onBegin).toHaveBeenCalledWith(
+        "http://foo.com/a.mp3",
+        8675309
+      );
+      expect(handlers.onDone).toHaveBeenCalledWith(
+        "http://foo.com/a.mp3",
+        `${RNFS.DocumentDirectoryPath}/DownloadQueue/mydomain/foo`
+      );
+    });
+
+    it("should restart download if finished spec has missing file", async () => {
+      const queue = new DownloadQueue();
+      const handlers: DownloadQueueHandlers = {
+        onBegin: jest.fn(),
+        onDone: jest.fn(),
+      };
+
+      (exists as jest.Mock).mockImplementation(() => true);
+      (stat as jest.Mock).mockImplementation(() => {
+        throw new Error("File not found");
+      });
+
+      await kvfs.write("/mydomain/foo", {
+        id: "foo",
+        url: "http://foo.com/a.mp3",
+        path: `${RNFS.DocumentDirectoryPath}/DownloadQueue/mydomain/foo`,
+        createTime: Date.now() - 1000,
+        finished: true,
+      });
+
+      await queue.init({ domain: "mydomain", handlers });
+
+      // Should restart download since file is missing
+      expect(download).toHaveBeenCalledTimes(1);
+
+      // Should not call handlers yet since file is missing
+      expect(handlers.onBegin).not.toHaveBeenCalled();
+      expect(handlers.onDone).not.toHaveBeenCalled();
+    });
+
     it("enforces netInfo callbacks when activeNetworkTypes is passed", async () => {
       const queue = new DownloadQueue();