lucaong / react-minisearch

React integration for the MiniSearch client side full-text search library
MIT License
45 stars 8 forks source link

Initial documents store problem #43

Closed friscolie-cc closed 3 months ago

friscolie-cc commented 3 months ago

Hi! @lucaong

Description

Steps to reproduce

Problem identified

lucaong commented 3 months ago

Hi @friscolie-cc , Thanks for opening your issue. I understand that this can be confusing, but it is by design.

The documents initially passed in as a prop will be correctly indexed as the component mounts, but will not be re-indexed if the value changes. Therefore, if the value is initially empty or null, and only afterwards takes a non-empty value, the index will remain empty. This is by design, as re-indexing the document collection when it changes is something that is better controlled directly by the developer by explicit use of MiniSearch methods (such as discard, remove, replace, add, addAll, etc.).

This is documented in the README:

The initial collection of documents to add to the index. Note: this is just the initial collection, and mutating this argument won't cause reindexing. To add or remove documents after initialization, use the functions add/addAll/remove/removeAll/discard, etc.

In other words, this will NOT WORK (documents will not be re-indexed when they change):

// !!! This will NOT work correctly if the documents change !!!
const MyComponent = () => {
  const [documents, setDocuments] = useState([])
  const { search, searchResults } = useMiniSearch(documents, miniSearchOptions)

  // ...rest of the component, invoking setDocuments
}

This version will work (but can be inefficient depending on the case, as it will re-index everything on every change, even if only a single document changes):

const MyComponent = () => {
  const [documents, setDocuments] = useState([])
  const { search, searchResults, addAll, removeAll } = useMiniSearch(documents, miniSearchOptions)

  // Clear the index and re-index all documents whenever they change
  useEffect(() => {
    removeAll()
    addAll(documents)
  }, [documents])

  // ...rest of the component, invoking setDocuments
}

The reason is to leave freedom to developers to manage re-indexing of changes the way that fits best their application. If react-minisearch forced a specific strategy to manage changes without knowledge of the specific app, it would only work well in some cases.

I hope this helps

friscolie-cc commented 3 months ago

Hi @friscolie-cc , Thanks for opening your issue. I understand that this can be confusing, but it is by design.

The documents initially passed in as a prop will be correctly indexed as the component mounts, but will not be re-indexed if the value changes. Therefore, if the value is initially empty or null, and only afterwards takes a non-empty value, the index will remain empty. This is by design, as re-indexing the document collection when it changes is something that is better controlled directly by the developer by explicit use of MiniSearch methods (such as discard, remove, replace, add, addAll, etc.).

This is documented in the README:

The initial collection of documents to add to the index. Note: this is just the initial collection, and mutating this argument won't cause reindexing. To add or remove documents after initialization, use the functions add/addAll/remove/removeAll/discard, etc.

In other words, this will NOT WORK (documents will not be re-indexed when they change):

// !!! This will NOT work correctly if the documents change !!!
const MyComponent = () => {
  const [documents, setDocuments] = useState([])
  const { search, searchResults } = useMiniSearch(documents, miniSearchOptions)

  // ...rest of the component, invoking setDocuments
}

This version will work (but can be inefficient depending on the case, as it will re-index everything on every change, even if only a single document changes):

const MyComponent = () => {
  const [documents, setDocuments] = useState([])
  const { search, searchResults, addAll, removeAll } = useMiniSearch(documents, miniSearchOptions)

  // Clear the index and re-index all documents whenever they change
  useEffect(() => {
    removeAll()
    addAll(documents)
  }, [documents])

  // ...rest of the component, invoking setDocuments
}

The reason is to leave freedom to developers to manage re-indexing of changes the way that fits best their application. If react-minisearch forced a specific strategy to manage changes without knowledge of the specific app, it would only work well in some cases.

I hope this helps

Thanks for the replies @lucaong !

So you mean, by design it is intended that when utils.miniSearch.documentCount === 0 this line utils.addAll(documents); will not inject the document into minisearch?

friscolie-cc commented 3 months ago

Because the example provided kinda ambiguous, it only works when you pass a const array of object data.

If so, probably need an enhancements on the docs about passing dynamic data 👍

lucaong commented 3 months ago

So you mean, by design it is intended that when utils.miniSearch.documentCount === 0 this line utils.addAll(documents); will not inject the document into minisearch?

No, addAll(documents) should always add the documents into the index.

Could you provide some example code that shows the issue? That would help me understand the problem.

friscolie-cc commented 3 months ago
  const faz = useMemo(
    () => groupBy(foo, 'bar'),
    [faz]
  );
  const [bar, setBar] = useState(faz);

  const workspaceDocuments = useMemo(
    () =>
      Object.entries(faz).flatMap(([_, bar]) =>
        bar.map(foo => ({
          name: foo.name,
          tenantName: foo.tenantName,
          id: foo.id,
        }))
      ),
    [faz]
  );

  const {search, searchResults, isIndexing, addAll, removeAll} = useMiniSearch(workspaceDocuments, {
    idField: 'id',
    fields: ['name', 'tenantName'],
    storeFields: ['id', 'name', 'tenantName'],
    searchOptions: {
      fuzzy: 0.3,
      prefix: true,
    },
  });

this is a minimal code that I can show you. So, in this case workspaceDocument doesn't get stored into minisearch and when I check the documentCount equal to zero.

I didn't use static data like how it was provided on example, probably this was the issues.

I hope this make things clear

lucaong commented 3 months ago

Is it possible that faz is initially empty? If so, workspaceDocuments will also be initially empty, and when it later changes to a non-empty collection it will NOT be reindexed (react-minisearch only uses the initial value).

Try adding this to your code:

useEffect(() => {
  removeAll()
  addAll(workspaceDocuments)
}, [workspaceDocuments])

Does that fix it?

friscolie-cc commented 3 months ago

Is it possible that faz is initially empty? If so, workspaceDocuments will also be initially empty, and when it later changes to a non-empty collection it will NOT be reindexed (react-minisearch only uses the initial value).

Try adding this to your code:

useEffect(() => {
  removeAll()
  addAll(workspaceDocuments)
}, [workspaceDocuments])

Does that fix it?

The data should not be empty because I render the component only when the data is ready, that's why I'm kinda curious about the initial document store.

Yup, I finally use this workaround to fix that thank you very much!.