Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: move search to Web Worker #469

Merged
merged 1 commit into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docusaurus-search-local/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
"@node-rs/jieba": "^1.6.0",
"cheerio": "^1.0.0",
"clsx": "^1.1.1",
"comlink": "^4.4.2",
"debug": "^4.2.0",
"fs-extra": "^10.0.0",
"klaw-sync": "^6.0.0",
Expand Down
24 changes: 14 additions & 10 deletions docusaurus-search-local/src/client/theme/SearchBar/SearchBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,11 @@ import {
} from "@docusaurus/theme-common";
import { useActivePlugin } from "@docusaurus/plugin-content-docs/client";

import { fetchIndexes } from "./fetchIndexes";
import { SearchSourceFactory } from "../../utils/SearchSourceFactory";
import { fetchIndexesByWorker, searchByWorker } from "../searchByWorker";
import { SuggestionTemplate } from "./SuggestionTemplate";
import { EmptyTemplate } from "./EmptyTemplate";
import { SearchResult } from "../../../shared/interfaces";
import {
searchResultLimits,
Mark,
searchBarShortcut,
searchBarShortcutHint,
Expand Down Expand Up @@ -149,9 +147,9 @@ export default function SearchBar({
search.current?.autocomplete.destroy();
setLoading(true);

const [{ wrappedIndexes, zhDictionary }, autoComplete] = await Promise.all([
fetchIndexes(versionUrl, searchContext),
const [autoComplete] = await Promise.all([
fetchAutoCompleteJS(),
fetchIndexesByWorker(versionUrl, searchContext),
]);

const searchFooterLinkElement = ({
Expand Down Expand Up @@ -256,11 +254,17 @@ export default function SearchBar({
},
[
{
source: SearchSourceFactory(
wrappedIndexes,
zhDictionary,
searchResultLimits
),
source: async (
input: string,
callback: (output: SearchResult[]) => void
) => {
const result = await searchByWorker(
versionUrl,
searchContext,
input
);
callback(result);
},
Comment on lines +257 to +267
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling to the asynchronous source function

The source function used in the autocomplete configuration is asynchronous but lacks error handling. If searchByWorker throws an error, it could lead to unhandled promise rejections, impacting the user experience.

Apply this diff to include error handling:

          source: async (
            input: string,
            callback: (output: SearchResult[]) => void
          ) => {
+           try {
              const result = await searchByWorker(
                versionUrl,
                searchContext,
                input
              );
              callback(result);
+           } catch (error) {
+             console.error("Error during search:", error);
+             callback([]);
+           }
          },

This ensures that any errors are caught, logged, and the autocomplete doesn't break.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
source: async (
input: string,
callback: (output: SearchResult[]) => void
) => {
const result = await searchByWorker(
versionUrl,
searchContext,
input
);
callback(result);
},
source: async (
input: string,
callback: (output: SearchResult[]) => void
) => {
try {
const result = await searchByWorker(
versionUrl,
searchContext,
input
);
callback(result);
} catch (error) {
console.error("Error during search:", error);
callback([]);
}
},

templates: {
suggestion: SuggestionTemplate,
empty: EmptyTemplate,
Expand Down

This file was deleted.

84 changes: 0 additions & 84 deletions docusaurus-search-local/src/client/theme/SearchBar/fetchIndexes.ts

This file was deleted.

43 changes: 21 additions & 22 deletions docusaurus-search-local/src/client/theme/SearchPage/SearchPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,7 @@ import { usePluralForm } from "@docusaurus/theme-common";
import clsx from "clsx";

import useSearchQuery from "../hooks/useSearchQuery";
import { fetchIndexes } from "../SearchBar/fetchIndexes";
import { SearchSourceFactory } from "../../utils/SearchSourceFactory";
import { fetchIndexesByWorker, searchByWorker } from "../searchByWorker";
import {
SearchDocument,
SearchDocumentType,
Expand Down Expand Up @@ -52,10 +51,6 @@ function SearchPageContent(): React.ReactElement {
updateSearchContext,
} = useSearchQuery();
const [searchQuery, setSearchQuery] = useState(searchValue);
const [searchSource, setSearchSource] =
useState<
(input: string, callback: (results: SearchResult[]) => void) => void
>();
const [searchResults, setSearchResults] = useState<SearchResult[]>();
const versionUrl = `${baseUrl}${searchVersion}`;

Expand Down Expand Up @@ -83,19 +78,22 @@ function SearchPageContent(): React.ReactElement {
useEffect(() => {
updateSearchPath(searchQuery);

if (searchSource) {
if (searchQuery) {
searchSource(searchQuery, (results) => {
setSearchResults(results);
});
} else {
setSearchResults(undefined);
}
if (searchQuery) {
(async () => {
const results = await searchByWorker(
versionUrl,
searchContext,
searchQuery
);
setSearchResults(results);
})();
} else {
setSearchResults(undefined);
Comment on lines +81 to +91
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor: Avoid defining async functions inside useEffect.

Defining an async function inside a useEffect hook can lead to unintentional side effects. Consider refactoring to define the async function outside the hook.

Apply this diff to refactor the code:

 useEffect(() => {
   updateSearchPath(searchQuery);

-  if (searchQuery) {
-    (async () => {
-      const results = await searchByWorker(
-        versionUrl,
-        searchContext,
-        searchQuery
-      );
-      setSearchResults(results);
-    })();
-  } else {
-    setSearchResults(undefined);
-  }
+  const fetchResults = async () => {
+    if (searchQuery) {
+      const results = await searchByWorker(
+        versionUrl,
+        searchContext,
+        searchQuery
+      );
+      setSearchResults(results);
+    } else {
+      setSearchResults(undefined);
+    }
+  };
+  fetchResults();
 }, [searchQuery, versionUrl, searchContext]);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (searchQuery) {
(async () => {
const results = await searchByWorker(
versionUrl,
searchContext,
searchQuery
);
setSearchResults(results);
})();
} else {
setSearchResults(undefined);
const fetchResults = async () => {
if (searchQuery) {
const results = await searchByWorker(
versionUrl,
searchContext,
searchQuery
);
setSearchResults(results);
} else {
setSearchResults(undefined);
}
};
fetchResults();

}

// `updateSearchPath` should not be in the deps,
// otherwise will cause call stack overflow.
}, [searchQuery, searchSource]);
}, [searchQuery, versionUrl, searchContext]);

const handleSearchInputChange = useCallback(
(e: React.ChangeEvent<HTMLInputElement>) => {
Expand All @@ -110,17 +108,18 @@ function SearchPageContent(): React.ReactElement {
}
}, [searchValue]);

const [searchWorkerReady, setSearchWorkerReady] = useState(false);

useEffect(() => {
async function doFetchIndexes() {
const { wrappedIndexes, zhDictionary } =
if (
!Array.isArray(searchContextByPaths) ||
searchContext ||
useAllContextsWithNoSearchContext
? await fetchIndexes(versionUrl, searchContext)
: { wrappedIndexes: [], zhDictionary: [] };
setSearchSource(() =>
SearchSourceFactory(wrappedIndexes, zhDictionary, 100)
);
) {
await fetchIndexesByWorker(versionUrl, searchContext);
}
setSearchWorkerReady(true);
Comment on lines +115 to +122
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add missing dependencies to the useEffect dependency array.

The variables searchContextByPaths and useAllContextsWithNoSearchContext are used inside the useEffect but are not included in the dependency array. To ensure the effect runs correctly when these variables change, include them in the dependencies.

Apply this diff to include the missing dependencies:

 useEffect(() => {
   async function doFetchIndexes() {
     if (
       !Array.isArray(searchContextByPaths) ||
       searchContext ||
       useAllContextsWithNoSearchContext
     ) {
       await fetchIndexesByWorker(versionUrl, searchContext);
     }
     setSearchWorkerReady(true);
   }
   doFetchIndexes();
-}, [searchContext, versionUrl]);
+}, [searchContext, versionUrl, searchContextByPaths, useAllContextsWithNoSearchContext]);

Committable suggestion skipped: line range outside the PR's diff.

}
doFetchIndexes();
}, [searchContext, versionUrl]);
Expand Down Expand Up @@ -198,7 +197,7 @@ function SearchPageContent(): React.ReactElement {
) : null}
</div>

{!searchSource && searchQuery && (
{!searchWorkerReady && searchQuery && (
<div>
<LoadingRing />
</div>
Expand Down
47 changes: 47 additions & 0 deletions docusaurus-search-local/src/client/theme/searchByWorker.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
import * as Comlink from "comlink";
import { SearchResult } from "../../shared/interfaces";

interface RemoteWorker {
fetchIndexes(baseUrl: string, searchContext: string): Promise<void>;
search(
baseUrl: string,
searchContext: string,
input: string
): Promise<SearchResult[]>;
}

let remoteWorkerPromise: Promise<RemoteWorker> | undefined;

function getRemoteWorker() {
if (!remoteWorkerPromise) {
remoteWorkerPromise = (async () => {
const Remote = Comlink.wrap(
new Worker(new URL("./worker.js", import.meta.url))
) as any;
return await new Remote();
})();
}
return remoteWorkerPromise;
}
Comment on lines +15 to +25
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for worker initialization

The worker initialization could fail for various reasons (e.g., worker file not found). Consider adding error handling.

 function getRemoteWorker() {
   if (!remoteWorkerPromise) {
     remoteWorkerPromise = (async () => {
-      const Remote = Comlink.wrap(
-        new Worker(new URL("./worker.js", import.meta.url))
-      ) as any;
-      return await new Remote();
+      try {
+        const Remote = Comlink.wrap(
+          new Worker(new URL("./worker.js", import.meta.url))
+        ) as Comlink.Remote<typeof RemoteWorker>;
+        return await new Remote();
+      } catch (error) {
+        console.error('Failed to initialize search worker:', error);
+        throw error;
+      }
     })();
   }
   return remoteWorkerPromise;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function getRemoteWorker() {
if (!remoteWorkerPromise) {
remoteWorkerPromise = (async () => {
const Remote = Comlink.wrap(
new Worker(new URL("./worker.js", import.meta.url))
) as any;
return await new Remote();
})();
}
return remoteWorkerPromise;
}
function getRemoteWorker() {
if (!remoteWorkerPromise) {
remoteWorkerPromise = (async () => {
try {
const Remote = Comlink.wrap(
new Worker(new URL("./worker.js", import.meta.url))
) as Comlink.Remote<typeof RemoteWorker>;
return await new Remote();
} catch (error) {
console.error('Failed to initialize search worker:', error);
throw error;
}
})();
}
return remoteWorkerPromise;
}


export async function fetchIndexesByWorker(
baseUrl: string,
searchContext: string
): Promise<void> {
if (process.env.NODE_ENV === "production") {
const remoteWorker = await getRemoteWorker();
await remoteWorker!.fetchIndexes(baseUrl, searchContext);
}
}
Comment on lines +27 to +35
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve error handling and return type consistency

The function could benefit from proper error handling and consistent return types.

 export async function fetchIndexesByWorker(
   baseUrl: string,
   searchContext: string
 ): Promise<void> {
   if (process.env.NODE_ENV === "production") {
-    const remoteWorker = await getRemoteWorker();
-    await remoteWorker!.fetchIndexes(baseUrl, searchContext);
+    try {
+      const remoteWorker = await getRemoteWorker();
+      await remoteWorker.fetchIndexes(baseUrl, searchContext);
+    } catch (error) {
+      console.error('Failed to fetch indexes:', error);
+      throw error;
+    }
+  } else {
+    // Explicitly return in non-production environment
+    return Promise.resolve();
   }
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export async function fetchIndexesByWorker(
baseUrl: string,
searchContext: string
): Promise<void> {
if (process.env.NODE_ENV === "production") {
const remoteWorker = await getRemoteWorker();
await remoteWorker!.fetchIndexes(baseUrl, searchContext);
}
}
export async function fetchIndexesByWorker(
baseUrl: string,
searchContext: string
): Promise<void> {
if (process.env.NODE_ENV === "production") {
try {
const remoteWorker = await getRemoteWorker();
await remoteWorker.fetchIndexes(baseUrl, searchContext);
} catch (error) {
console.error('Failed to fetch indexes:', error);
throw error;
}
} else {
// Explicitly return in non-production environment
return Promise.resolve();
}
}


export async function searchByWorker(
baseUrl: string,
searchContext: string,
input: string
): Promise<SearchResult[]> {
if (process.env.NODE_ENV === "production") {
const remoteWorker = await getRemoteWorker();
return remoteWorker!.search(baseUrl, searchContext, input);
}
return [];
}
Comment on lines +37 to +47
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add error handling for search operation

Similar to fetchIndexesByWorker, this function should handle potential errors gracefully.

 export async function searchByWorker(
   baseUrl: string,
   searchContext: string,
   input: string
 ): Promise<SearchResult[]> {
   if (process.env.NODE_ENV === "production") {
-    const remoteWorker = await getRemoteWorker();
-    return remoteWorker!.search(baseUrl, searchContext, input);
+    try {
+      const remoteWorker = await getRemoteWorker();
+      return await remoteWorker.search(baseUrl, searchContext, input);
+    } catch (error) {
+      console.error('Search operation failed:', error);
+      return [];
+    }
   }
   return [];
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export async function searchByWorker(
baseUrl: string,
searchContext: string,
input: string
): Promise<SearchResult[]> {
if (process.env.NODE_ENV === "production") {
const remoteWorker = await getRemoteWorker();
return remoteWorker!.search(baseUrl, searchContext, input);
}
return [];
}
export async function searchByWorker(
baseUrl: string,
searchContext: string,
input: string
): Promise<SearchResult[]> {
if (process.env.NODE_ENV === "production") {
try {
const remoteWorker = await getRemoteWorker();
return await remoteWorker.search(baseUrl, searchContext, input);
} catch (error) {
console.error('Search operation failed:', error);
return [];
}
}
return [];
}

Loading
Loading