Return stable references from useMiniSearch() hook (#31)

* test(useMiniSearch): add POC test

- This new test with minimal changes to the usual setup
demonstrates how bugs can be introduced in a complex
React project due to the useMiniSearch() hook
returning newly created util functions instead of stable
references after each render.

* feat(useMiniSearch): return stable fn references

* feat(useOnMount): remove eslint warning ignore and add useEffect cleanup instead

* Fix utils after merge

Co-authored-by: neezurft <contact@embeddednotes.com>
Co-authored-by: Luca Ongaro <mail@lucaongaro.eu>

src/react-minisearch.test.tsx

@@ -1,7 +1,7 @@
 /* eslint-env jest */
 
 import { mount } from 'enzyme'
-import React, { ChangeEvent } from 'react'
+import React, { ChangeEvent, useEffect } from 'react'
 import { act } from 'react-dom/test-utils'
 import { useMiniSearch, withMiniSearch, WithMiniSearch, UseMiniSearch } from './react-minisearch'
 import MiniSearch, { Options } from 'minisearch'
@@ -290,6 +290,26 @@ describe('useMiniSearch', () => {
   }
 
   testComponent(MyComponent)
+
+  describe('does not trigger unintended effects', () => {
+    const mockDocumentsFetch = () => ({
+      then: (cb) => cb(documents)
+    })
+
+    const MyComponent = ({ options }) => {
+      const props = useMiniSearch<DocumentType>([], options)
+
+      const { addAll } = props
+
+      useEffect(() => {
+        mockDocumentsFetch().then(addAll)
+      }, [addAll])
+
+      return <ChildComponent {...props} />
+    }
+
+    testComponent(MyComponent)
+  })
 })
 
 describe('withMiniSearch', () => {

src/react-minisearch.tsx

@@ -1,5 +1,5 @@
 import MiniSearch, { Query, Options, SearchOptions, SearchResult, Suggestion } from 'minisearch'
-import React, { useEffect, useState, useRef, PropsWithChildren } from 'react'
+import React, { useEffect, useState, useRef, PropsWithChildren, useMemo } from 'react'
 
 export interface UseMiniSearch<T = any> {
   search: (query: Query, options?: SearchOptions) => void,
@@ -23,119 +23,143 @@ export interface UseMiniSearch<T = any> {
 }
 
 export function useMiniSearch<T = any> (documents: T[], options: Options<T>): UseMiniSearch<T> {
+  const optionsRef = useRef(options)
   const miniSearchRef = useRef<MiniSearch<T>>(new MiniSearch<T>(options))
+  const documentByIdRef = useRef<{ [key: string]: T }>({})
+
   const [rawResults, setRawResults] = useState<SearchResult[] | null>(null)
   const [searchResults, setSearchResults] = useState<T[] | null>(null)
   const [suggestions, setSuggestions] = useState<Suggestion[] | null>(null)
-  const documentByIdRef = useRef<{ [key: string]: T }>({})
   const [isIndexing, setIsIndexing] = useState<boolean>(false)
-  const idField = options.idField || MiniSearch.getDefault('idField') as Options['idField']
-  const extractField = options.extractField || MiniSearch.getDefault('extractField') as Options['extractField']
-  const gatherById = (documents) => documents.reduce((byId, doc) => {
-    const id = extractField(doc, idField)
-    byId[id] = doc
-    return byId
-  }, {})
-
-  const miniSearch = miniSearchRef.current
-  const documentById = documentByIdRef.current
-
-  const search = (query: string, searchOptions?: SearchOptions): void => {
-    const results = miniSearch.search(query, searchOptions)
-    const searchResults = results.map(({ id }) => documentById[id])
-    setSearchResults(searchResults)
-    setRawResults(results)
-  }
 
-  const autoSuggest = (query: string, searchOptions?: SearchOptions): void => {
-    const suggestions = miniSearch.autoSuggest(query, searchOptions)
-    setSuggestions(suggestions)
-  }
+  const utils = useMemo(() => {
+    const miniSearch = miniSearchRef.current
+    const documentById = documentByIdRef.current
+    const options = optionsRef.current
+
+    const idField = options.idField || MiniSearch.getDefault('idField') as Options['idField']
+    const extractField = options.extractField || MiniSearch.getDefault('extractField') as Options['extractField']
+    const gatherById = (documents) => documents.reduce((byId, doc) => {
+      const id = extractField(doc, idField)
+      byId[id] = doc
+      return byId
+    }, {})
+
+    const search = (query: string, searchOptions?: SearchOptions): void => {
+      const results = miniSearch.search(query, searchOptions)
+      const searchResults = results.map(({ id }) => documentById[id])
+      setSearchResults(searchResults)
+      setRawResults(results)
+    }
 
-  const add = (document: T): void => {
-    documentByIdRef.current[extractField(document, idField)] = document
-    miniSearch.add(document)
-  }
+    const autoSuggest = (query: string, searchOptions?: SearchOptions): void => {
+      const suggestions = miniSearch.autoSuggest(query, searchOptions)
+      setSuggestions(suggestions)
+    }
 
-  const addAll = (documents: readonly T[]): void => {
-    const byId = gatherById(documents)
-    documentByIdRef.current = Object.assign(documentById, byId)
-    miniSearch.addAll(documents)
-  }
+    const add = (document: T): void => {
+      documentByIdRef.current[extractField(document, idField)] = document
+      miniSearch.add(document)
+    }
 
-  const addAllAsync = (documents: readonly T[], options?: { chunkSize?: number }): Promise<void> => {
-    const byId = gatherById(documents)
-    documentByIdRef.current = Object.assign(documentById, byId)
-    setIsIndexing(true)
+    const addAll = (documents: readonly T[]): void => {
+      const byId = gatherById(documents)
+      documentByIdRef.current = Object.assign(documentById, byId)
+      miniSearch.addAll(documents)
+    }
 
-    return miniSearch.addAllAsync(documents, options || {}).then(() => {
-      setIsIndexing(false)
-    })
-  }
+    const addAllAsync = (documents: readonly T[], options?: { chunkSize?: number }): Promise<void> => {
+      const byId = gatherById(documents)
+      documentByIdRef.current = Object.assign(documentById, byId)
+      setIsIndexing(true)
 
-  const remove = (document: T): void => {
-    miniSearch.remove(document)
-    documentByIdRef.current = removeFromMap<T>(documentById, extractField(document, idField))
-  }
+      return miniSearch.addAllAsync(documents, options || {}).then(() => {
+        setIsIndexing(false)
+      })
+    }
 
-  const removeAll = function (documents?: readonly T[]): void {
-    if (arguments.length === 0) {
-      miniSearch.removeAll()
-      documentByIdRef.current = {}
-    } else {
-      miniSearch.removeAll(documents)
-      const idsToRemove = documents.map((doc) => extractField(doc, idField))
-      documentByIdRef.current = removeManyFromMap<T>(documentById, idsToRemove)
+    const remove = (document: T): void => {
+      miniSearch.remove(document)
+      documentByIdRef.current = removeFromMap<T>(documentById, extractField(document, idField))
     }
-  }
 
-  const discard = (id: any): void => {
-    miniSearch.discard(id)
-    documentByIdRef.current = removeFromMap<T>(documentById, id)
-  }
+    const removeById = (id: any): void => {
+      const document = documentById[id]
+      if (document == null) {
+        throw new Error(`react-minisearch: document with id ${id} does not exist in the index`)
+      }
+      miniSearch.remove(document)
+      documentByIdRef.current = removeFromMap<T>(documentById, id)
+    }
 
-  const discardAll = (ids: readonly any[]): void => {
-    miniSearch.discardAll(ids)
-    documentByIdRef.current = removeManyFromMap<T>(documentById, ids)
-  }
+    const removeAll = function (documents?: readonly T[]): void {
+      if (arguments.length === 0) {
+        miniSearch.removeAll()
+        documentByIdRef.current = {}
+      } else {
+        miniSearch.removeAll(documents)
+        const idsToRemove = documents.map((doc) => extractField(doc, idField))
+        documentByIdRef.current = removeManyFromMap<T>(documentById, idsToRemove)
+      }
+    }
 
-  const replace = (document: T): void => {
-    miniSearch.replace(document)
-    documentByIdRef.current[extractField(document, idField)] = document
-  }
+    const discard = (id: any): void => {
+      miniSearch.discard(id)
+      documentByIdRef.current = removeFromMap<T>(documentById, id)
+    }
 
-  const clearSearch = (): void => {
-    setSearchResults(null)
-    setRawResults(null)
-  }
+    const discardAll = (ids: readonly any[]): void => {
+      miniSearch.discardAll(ids)
+      documentByIdRef.current = removeManyFromMap<T>(documentById, ids)
+    }
 
-  const clearSuggestions = (): void => {
-    setSuggestions(null)
-  }
+    const replace = (document: T): void => {
+      miniSearch.replace(document)
+      documentByIdRef.current[extractField(document, idField)] = document
+    }
 
-  useOnMount(() => {
-    addAll(documents)
-  })
+    const clearSearch = (): void => {
+      setSearchResults(null)
+      setRawResults(null)
+    }
+
+    const clearSuggestions = (): void => {
+      setSuggestions(null)
+    }
+
+    return {
+      search,
+      autoSuggest,
+      add,
+      addAll,
+      addAllAsync,
+      remove,
+      removeById,
+      removeAll,
+      discard,
+      discardAll,
+      replace,
+      clearSearch,
+      clearSuggestions,
+      miniSearch
+    }
+  }, [])
+
+  useEffect(() => {
+    utils.addAll(documents)
+
+    return () => {
+      utils.removeAll(documents)
+    }
+  }, [utils, documents])
 
   return {
-    search,
     searchResults,
     rawResults,
-    autoSuggest,
     suggestions,
-    add,
-    addAll,
-    addAllAsync,
-    remove,
-    removeById: discard,
-    removeAll,
-    discard,
-    discardAll,
-    replace,
     isIndexing,
-    clearSearch,
-    clearSuggestions,
-    miniSearch
+    ...utils,
+    removeById: utils.discard
   }
 }
 
@@ -155,11 +179,6 @@ function getDisplayName<PropsT> (Component: React.ComponentType<PropsT>): string
   return Component.displayName || Component.name || 'Component'
 }
 
-function useOnMount (callback: React.EffectCallback) {
-  // eslint-disable-next-line react-hooks/exhaustive-deps
-  return useEffect(callback, [])
-}
-
 export function withMiniSearch<OwnProps, T = any> (
   documents: T[],
   options: Options<T>,