fix: prevent to load script again (#756)

* fix: prevent to load script again * simplify * test: add federation loadScript * fix: remove finished promises * test: add a complex loadScript test * fix: return promise if exist * test: combine prefetchScript with loadScript * fix: should run loadScript if promise is create because of prefetch * chore: changeset * chore: lint * chore: simplify code * Update packages/repack/src/modules/ScriptManager/__tests__/Federated.test.ts Co-authored-by: Jakub Romańczyk <[email protected]> * move tests to scriptManager test * fix: script promise type * fix: spyOn loadScript
callstack · Oct 17, 2024 · f119ab3 · f119ab3
1 parent 55131d8
commit f119ab3
Show file tree

Hide file tree

Showing 3 changed files with 186 additions and 29 deletions.
diff --git a/.changeset/olive-singers-attack.md b/.changeset/olive-singers-attack.md
@@ -0,0 +1,6 @@
+---
+'@callstack/repack': patch
+---
+
+Prevent to loadScript which is already is loading
+issue: https://github.com/callstack/repack/issues/749
diff --git a/packages/repack/src/modules/ScriptManager/ScriptManager.ts b/packages/repack/src/modules/ScriptManager/ScriptManager.ts
@@ -12,6 +12,11 @@ type Cache = Record<
   Pick<NormalizedScriptLocator, 'method' | 'url' | 'query' | 'headers' | 'body'>
 >;
 
+type ScriptsPromises = Record<
+  string,
+  (Promise<void> & { isPrefetch?: true }) | undefined
+>;
+
 const CACHE_NAME = 'Repack.ScriptManager.Cache';
 const CACHE_VERSION = 'v4';
 const CACHE_ENV = __DEV__ ? 'debug' : 'release';
@@ -100,6 +105,7 @@ export class ScriptManager extends EventEmitter {
   }
 
   protected cache: Cache = {};
+  protected scriptsPromises: ScriptsPromises = {};
   protected cacheInitialized = false;
   protected resolvers: [number, ScriptLocatorResolver][] = [];
   protected storage?: StorageApi;
@@ -320,21 +326,39 @@ export class ScriptManager extends EventEmitter {
     caller?: string,
     webpackContext = getWebpackContext()
   ) {
-    const script = await this.resolveScript(scriptId, caller, webpackContext);
-
-    try {
-      this.emit('loading', script.toObject());
-      await this.loadScriptWithRetry(scriptId, script.locator);
-      this.emit('loaded', script.toObject());
-    } catch (error) {
-      const { code } = error as Error & { code: string };
-      this.handleError(
-        error,
-        '[ScriptManager] Failed to load script:',
-        code ? `[${code}]` : '',
-        script.toObject()
-      );
+    const uniqueId = Script.getScriptUniqueId(scriptId, caller);
+    if (this.scriptsPromises[uniqueId]) {
+      const { isPrefetch } = this.scriptsPromises[uniqueId];
+
+      // prefetch is not execute the script so we need to run loadScript if promise is for prefetch
+      if (isPrefetch) {
+        await this.scriptsPromises[uniqueId];
+      } else {
+        return this.scriptsPromises[uniqueId];
+      }
     }
+    const loadProcess = async () => {
+      const script = await this.resolveScript(scriptId, caller, webpackContext);
+
+      try {
+        this.emit('loading', script.toObject());
+        await this.loadScriptWithRetry(scriptId, script.locator);
+        this.emit('loaded', script.toObject());
+      } catch (error) {
+        const { code } = error as Error & { code: string };
+        this.handleError(
+          error,
+          '[ScriptManager] Failed to load script:',
+          code ? `[${code}]` : '',
+          script.toObject()
+        );
+      }
+
+      delete this.scriptsPromises[uniqueId];
+    };
+
+    this.scriptsPromises[uniqueId] = loadProcess();
+    return this.scriptsPromises[uniqueId];
   }
 
   /**
@@ -390,20 +414,32 @@ export class ScriptManager extends EventEmitter {
     caller?: string,
     webpackContext = getWebpackContext()
   ) {
-    const script = await this.resolveScript(scriptId, caller, webpackContext);
-
-    try {
-      this.emit('prefetching', script.toObject());
-      await this.nativeScriptManager.prefetchScript(scriptId, script.locator);
-    } catch (error) {
-      const { code } = error as Error & { code: string };
-      this.handleError(
-        error,
-        '[ScriptManager] Failed to prefetch script:',
-        code ? `[${code}]` : '',
-        script.toObject()
-      );
+    const uniqueId = Script.getScriptUniqueId(scriptId, caller);
+    if (this.scriptsPromises[uniqueId]) {
+      return this.scriptsPromises[uniqueId];
     }
+    const loadProcess = async () => {
+      const script = await this.resolveScript(scriptId, caller, webpackContext);
+
+      try {
+        this.emit('prefetching', script.toObject());
+        await this.nativeScriptManager.prefetchScript(scriptId, script.locator);
+      } catch (error) {
+        const { code } = error as Error & { code: string };
+        this.handleError(
+          error,
+          '[ScriptManager] Failed to prefetch script:',
+          code ? `[${code}]` : '',
+          script.toObject()
+        );
+      }
+
+      delete this.scriptsPromises[uniqueId];
+    };
+
+    this.scriptsPromises[uniqueId] = loadProcess();
+    this.scriptsPromises[uniqueId].isPrefetch = true;
+    return this.scriptsPromises[uniqueId];
   }
 
   /**
@@ -422,7 +458,10 @@ export class ScriptManager extends EventEmitter {
       await this.initCache();
 
       const ids = scriptIds.length ? scriptIds : Object.keys(this.cache);
-      ids.forEach((scriptId) => delete this.cache[scriptId]);
+      ids.forEach((scriptId) => {
+        delete this.cache[scriptId];
+        delete this.scriptsPromises[scriptId];
+      });
 
       await this.saveCache();
       await this.nativeScriptManager.invalidateScripts(scriptIds);

diff --git a/packages/repack/src/modules/ScriptManager/__tests__/ScriptManager.test.ts b/packages/repack/src/modules/ScriptManager/__tests__/ScriptManager.test.ts
@@ -1,4 +1,6 @@
-import NativeScriptManager from '../NativeScriptManager';
+import NativeScriptManager, {
+  type NormalizedScriptLocator,
+} from '../NativeScriptManager';
 import { Script } from '../Script';
 import { ScriptManager } from '../ScriptManager';
 
@@ -736,4 +738,114 @@ describe('ScriptManagerAPI', () => {
     expect(NativeScriptManager.loadScript).toHaveBeenCalledTimes(3);
     jest.useRealTimers();
   });
+
+  it('should await loadScript with same scriptId to finish', async () => {
+    const spy = mockLoadScriptBasedOnFetch();
+
+    const cache = new FakeCache();
+    ScriptManager.shared.setStorage(cache);
+
+    ScriptManager.shared.addResolver(async (scriptId, _caller) => {
+      return {
+        url: Script.getRemoteURL(scriptId),
+        cache: true,
+      };
+    });
+
+    let loadingScriptIsFinished = false;
+
+    // loadScript should wait first time called loadScript although we are not awaited, because scriptId is same
+    ScriptManager.shared.loadScript('miniApp').then(() => {
+      loadingScriptIsFinished = true;
+    });
+    await ScriptManager.shared.loadScript('miniApp');
+
+    expect(loadingScriptIsFinished).toEqual(true);
+    spy.mockReset();
+    spy.mockRestore();
+  });
+
+  it('should wait loadScript with same scriptId to finished in a complex scenario', async () => {
+    const spy = mockLoadScriptBasedOnFetch();
+
+    const cache = new FakeCache();
+    ScriptManager.shared.setStorage(cache);
+
+    ScriptManager.shared.addResolver(async (scriptId, _caller) => {
+      return {
+        url: Script.getRemoteURL(scriptId),
+        cache: true,
+      };
+    });
+
+    let loadingScriptIsFinished = false;
+    let loadingScript2IsFinished = false;
+
+    // loadScript should wait first time called loadScript although we are not awaited, because scriptId is same
+    ScriptManager.shared.loadScript('miniApp').then(() => {
+      loadingScriptIsFinished = true;
+    });
+
+    ScriptManager.shared.loadScript('miniApp2').then(() => {
+      loadingScript2IsFinished = true;
+    });
+    await ScriptManager.shared.loadScript('miniApp');
+    expect(loadingScriptIsFinished).toEqual(true);
+
+    loadingScriptIsFinished = false;
+    ScriptManager.shared.loadScript('miniApp').then(() => {
+      loadingScriptIsFinished = true;
+    });
+
+    ScriptManager.shared.loadScript('miniApp2');
+    await ScriptManager.shared.loadScript('miniApp');
+
+    expect(loadingScriptIsFinished).toEqual(true);
+    await ScriptManager.shared.loadScript('miniApp2');
+    expect(loadingScript2IsFinished).toEqual(true);
+    spy.mockReset();
+    spy.mockRestore();
+  });
+
+  it('should wait loadScript and prefetchScript', async () => {
+    const spy = mockLoadScriptBasedOnFetch();
+
+    const cache = new FakeCache();
+    ScriptManager.shared.setStorage(cache);
+
+    ScriptManager.shared.addResolver(async (scriptId, _caller) => {
+      return {
+        url: Script.getRemoteURL(scriptId),
+        cache: true,
+      };
+    });
+
+    let prefetchScriptIsFinished = false;
+
+    // loadScript should wait first time called loadScript although we are not awaited, because scriptId is same
+    ScriptManager.shared.prefetchScript('miniApp').then(() => {
+      prefetchScriptIsFinished = true;
+    });
+    await ScriptManager.shared.loadScript('miniApp');
+
+    expect(prefetchScriptIsFinished).toEqual(true);
+    spy.mockReset();
+    spy.mockRestore();
+  });
 });
+
+function mockLoadScriptBasedOnFetch() {
+  jest.useFakeTimers({ advanceTimers: true });
+  const spy = jest.spyOn(NativeScriptManager, 'loadScript');
+
+  spy.mockImplementation(
+    (_scriptId: string, scriptConfig: NormalizedScriptLocator) =>
+      scriptConfig.fetch
+        ? new Promise<null>((resolve) => {
+            setTimeout(() => resolve(null), 10);
+          })
+        : Promise.resolve(null)
+  );
+
+  return spy;
+}