fix: don't cache failed script

.changeset/tricky-donuts-fly.md

@@ -0,0 +1,5 @@
+---
+"@callstack/repack": patch
+---
+
+script should be cached after successfully loaded

packages/repack/src/modules/ScriptManager/ScriptManager.ts

@@ -275,8 +275,6 @@ export class ScriptManager extends EventEmitter {
         // If it returns true, we need to fetch the script
         if (fetch) {
           script.locator.fetch = true;
-          this.cache[cacheKey] = script.getCacheData();
-          await this.saveCache();
         }
 
         this.emit('resolved', script.toObject());
@@ -288,12 +286,8 @@ export class ScriptManager extends EventEmitter {
       // If no custom shouldUpdateScript function was provided, we use the default behaviour
       if (!this.cache[cacheKey]) {
         script.locator.fetch = true;
-        this.cache[cacheKey] = script.getCacheData();
-        await this.saveCache();
       } else if (script.shouldRefetch(this.cache[cacheKey])) {
         script.locator.fetch = true;
-        this.cache[cacheKey] = script.getCacheData();
-        await this.saveCache();
       }
 
       this.emit('resolved', script.toObject());
@@ -308,6 +302,14 @@ export class ScriptManager extends EventEmitter {
     }
   }
 
+  private async updateCache(script: Script) {
+    if (script.locator.fetch) {
+      const cacheKey = script.locator.uniqueId;
+      this.cache[cacheKey] = script.getCacheData();
+      await this.saveCache();
+    }
+  }
+
   /**
    * Resolves given script's location, downloads and executes it.
    * The execution of the code is handled internally by threading in React Native.
@@ -344,6 +346,7 @@ export class ScriptManager extends EventEmitter {
         this.emit('loading', script.toObject());
         await this.loadScriptWithRetry(scriptId, script.locator);
         this.emit('loaded', script.toObject());
+        await this.updateCache(script);
       } catch (error) {
         const { code } = error as Error & { code: string };
         this.handleError(
@@ -352,9 +355,10 @@ export class ScriptManager extends EventEmitter {
           code ? `[${code}]` : '',
           script.toObject()
         );
+      } finally {
+        // should delete script promise even script failed
+        delete this.scriptsPromises[uniqueId];
       }
-
-      delete this.scriptsPromises[uniqueId];
     };
 
     this.scriptsPromises[uniqueId] = loadProcess();
@@ -424,6 +428,7 @@ export class ScriptManager extends EventEmitter {
       try {
         this.emit('prefetching', script.toObject());
         await this.nativeScriptManager.prefetchScript(scriptId, script.locator);
+        await this.updateCache(script);
       } catch (error) {
         const { code } = error as Error & { code: string };
         this.handleError(
@@ -432,9 +437,10 @@ export class ScriptManager extends EventEmitter {
           code ? `[${code}]` : '',
           script.toObject()
         );
+      } finally {
+        // should delete script promise even script failed
+        delete this.scriptsPromises[uniqueId];
       }
-
-      delete this.scriptsPromises[uniqueId];
     };
 
     this.scriptsPromises[uniqueId] = loadProcess();

packages/repack/src/modules/ScriptManager/__tests__/ScriptManager.test.ts

@@ -183,6 +183,8 @@ describe('ScriptManagerAPI', () => {
       uniqueId: 'main_src_App_js',
     });
 
+    await ScriptManager.shared.loadScript('src_App_js', 'main');
+
     const {
       locator: { fetch },
     } = await ScriptManager.shared.resolveScript('src_App_js', 'main');
@@ -435,6 +437,7 @@ describe('ScriptManagerAPI', () => {
       'src_App_js',
       'main'
     );
+    await ScriptManager.shared.loadScript('src_App_js', 'main');
     expect(script1.locator.fetch).toBe(false);
 
     ScriptManager.shared.removeAllResolvers();
@@ -454,6 +457,7 @@ describe('ScriptManagerAPI', () => {
       'src_App_js',
       'main'
     );
+    await ScriptManager.shared.loadScript('src_App_js', 'main');
     expect(script2.locator.fetch).toBe(true);
 
     ScriptManager.shared.removeAllResolvers();
@@ -473,6 +477,7 @@ describe('ScriptManagerAPI', () => {
       'src_App_js',
       'main'
     );
+    await ScriptManager.shared.loadScript('src_App_js', 'main');
     expect(script3.locator.fetch).toBe(true);
 
     ScriptManager.shared.removeAllResolvers();
@@ -496,6 +501,7 @@ describe('ScriptManagerAPI', () => {
       'src_App_js',
       'main'
     );
+    await ScriptManager.shared.loadScript('src_App_js', 'main');
     expect(script4.locator.fetch).toBe(true);
 
     ScriptManager.shared.removeAllResolvers();
@@ -519,6 +525,7 @@ describe('ScriptManagerAPI', () => {
       'src_App_js',
       'main'
     );
+    await ScriptManager.shared.loadScript('src_App_js', 'main');
     expect(script5.locator.fetch).toBe(false);
 
     ScriptManager.shared.removeAllResolvers();
@@ -590,7 +597,7 @@ describe('ScriptManagerAPI', () => {
     expect(NativeScriptManager.loadScript).toHaveBeenCalledTimes(1);
     expect(NativeScriptManager.loadScript).toHaveBeenCalledWith(scriptId, {
       absolute: false,
-      fetch: false,
+      fetch: true,
       method: 'GET',
       retry: 2,
       retryDelay: 100,
@@ -646,7 +653,7 @@ describe('ScriptManagerAPI', () => {
     expect(NativeScriptManager.loadScript).toHaveBeenCalledTimes(3);
     expect(NativeScriptManager.loadScript).toHaveBeenCalledWith(scriptId, {
       absolute: false,
-      fetch: false,
+      fetch: true,
       method: 'GET',
       retry: 2,
       retryDelay: 100,
@@ -761,7 +768,7 @@ describe('ScriptManagerAPI', () => {
     await ScriptManager.shared.loadScript('miniApp');
 
     expect(loadingScriptIsFinished).toEqual(true);
-    spy.mockReset();
+
     spy.mockRestore();
   });
 
@@ -803,7 +810,7 @@ describe('ScriptManagerAPI', () => {
     expect(loadingScriptIsFinished).toEqual(true);
     await ScriptManager.shared.loadScript('miniApp2');
     expect(loadingScript2IsFinished).toEqual(true);
-    spy.mockReset();
+
     spy.mockRestore();
   });
 
@@ -829,7 +836,41 @@ describe('ScriptManagerAPI', () => {
     await ScriptManager.shared.loadScript('miniApp');
 
     expect(prefetchScriptIsFinished).toEqual(true);
-    spy.mockReset();
+
+    spy.mockRestore();
+  });
+
+  it('should refetch failed script', async () => {
+    jest.useFakeTimers({ advanceTimers: true });
+    const spy = jest.spyOn(NativeScriptManager, 'loadScript');
+
+    spy.mockRejectedValueOnce(
+      (_scriptId: string, _scriptConfig: NormalizedScriptLocator) =>
+        Promise.reject({ code: 'NetworkFailed' })
+    );
+
+    const cache = new FakeCache();
+    ScriptManager.shared.setStorage(cache);
+
+    ScriptManager.shared.addResolver(async (scriptId, _caller) => {
+      return {
+        url: Script.getRemoteURL(scriptId),
+        cache: true,
+      };
+    });
+
+    await expect(async () =>
+      ScriptManager.shared.loadScript('miniApp')
+    ).rejects.toThrow();
+
+    // //expected to cache be empty
+    expect(Object.keys(cache.data).length).toBe(0);
+
+    await ScriptManager.shared.loadScript('miniApp');
+
+    // expected to fetch again
+    expect(spy.mock.lastCall?.[1].fetch).toBe(true);
+
     spy.mockRestore();
   });
 });