-
Notifications
You must be signed in to change notification settings - Fork 94
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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, | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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}`; | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Refactor: Avoid defining async functions inside Defining an async function inside a 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
// `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>) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add missing dependencies to the The variables 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]);
|
||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||
doFetchIndexes(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
}, [searchContext, versionUrl]); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -198,7 +197,7 @@ function SearchPageContent(): React.ReactElement { | |||||||||||||||||||||||||||||||||||||||||||||||||
) : null} | ||||||||||||||||||||||||||||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||
{!searchSource && searchQuery && ( | ||||||||||||||||||||||||||||||||||||||||||||||||||
{!searchWorkerReady && searchQuery && ( | ||||||||||||||||||||||||||||||||||||||||||||||||||
<div> | ||||||||||||||||||||||||||||||||||||||||||||||||||
<LoadingRing /> | ||||||||||||||||||||||||||||||||||||||||||||||||||
</div> | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+27
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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 []; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+37
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add error handling to the asynchronous
source
functionThe
source
function used in the autocomplete configuration is asynchronous but lacks error handling. IfsearchByWorker
throws an error, it could lead to unhandled promise rejections, impacting the user experience.Apply this diff to include error handling:
This ensures that any errors are caught, logged, and the autocomplete doesn't break.
📝 Committable suggestion