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

getRefinements can receive null and system crashes when it does. #6441

Closed
1 task done
mmorris8 opened this issue Nov 19, 2024 · 8 comments · May be fixed by #6442
Closed
1 task done

getRefinements can receive null and system crashes when it does. #6441

mmorris8 opened this issue Nov 19, 2024 · 8 comments · May be fixed by #6442
Labels
triage Issues to be categorized by the team

Comments

@mmorris8
Copy link

mmorris8 commented Nov 19, 2024

🐛 Current behavior

File: /node_modules/instantsearch.js/es/lib/utils/getRefinements

Function getRefinements...

export function getRefinements(results, state) {
  var includesQuery = arguments.length > 2 && arguments[2] !== undefined ? arguments[2] : false;
  var refinements = [];
  var _state$facetsRefineme = state.facetsRefinements,

results can be null. This function crashes when it is as there is no guard in the function for this possibility. Adding a guard and returning an empty array (as in, no refinements possible) the error goes away.

export function getRefinements(results, state) {
  if (results === null) return []
  var includesQuery = arguments.length > 2 && arguments[2] !== undefined ? arguments[2] : false;
  var refinements = [];
  var _state$facetsRefineme = state.facetsRefinements,

🔍 Steps to reproduce

I have no clue how my code is causing a null object to be passed to this function. I just know that it is happening and a straightforward hack stops the nonsense.

Live reproduction

No sandbox should be necessary - and I honestly have no clue on how to reproduce the bug in a simplified example.

💭 Expected behavior

Throwing an unguarded error is a bug. The API should either do the above to recover, or throw a detailed error explaining what I can do in my code to prevent a null object from getting passed into this function.

Package version

instantsearch 4.75.4

Operating system

OSX Sequoia

Browser

Edge Latest

Code of Conduct

  • I agree to follow this project's Code of Conduct
@mmorris8 mmorris8 added the triage Issues to be categorized by the team label Nov 19, 2024
Haroenv added a commit that referenced this issue Nov 19, 2024
In some cases `results` in `render` can be null:
- no index name passed
- stalled render

In the vast majority of cases we already handled `results` being `null` or `undefined` just in case, but now i've changed the type to ensure we always catch this problem

This issue doesn't have a reproduction, but i believe it now guards for all possible cases of "no results", so:
fixes #6441

[CR-7353]
@Haroenv
Copy link
Contributor

Haroenv commented Nov 19, 2024

Thanks for catching this, I believe the case you're ending up may be when you're having a widget that reads results (current refinements, or clear refinements probably in this case) in a root index without index name passed. The error is confusing so I've opened a PR to guard for that case and ensure that we can't have other cases of this behaviour

@mmorris8
Copy link
Author

mmorris8 commented Nov 19, 2024

Hmm. I'm using a tabbed multi-index search were the query is always visible, but it isn't associated to any of the child indexes. So I'll guess that's the source point of the error. The indexes being searched are somewhat unrelated so I didn't set a master indexName on the InstantSearch object itself, only specifying them on the children. I'll go ahead and set one and see if the error goes away and if the presence of the keyword doesn't break anything.

My pattern is therefore something like this

<InstantSearch>/*no indexName specified here */
  <SearchMast /> /* The query box is inside this component */
  <Index indexName="provider">Provider searching - additional filters specific to providers here</Index>
  <Index indexName="location">Location searches - additional filters specific to locations here</Index>
  <Index indexName="page">Page searches</Index>
</InstantSearch>

@Haroenv
Copy link
Contributor

Haroenv commented Nov 19, 2024

Yes, this verifies that the pull request would fix your use case. What you can do to work around it as well is pass a dummy index name, and filter that out in the search method (and put a fake result in the response).

In SearchMast do you only have search box, or other widgets too?

@mmorris8
Copy link
Author

I used to have the Location control up there but I've since moved it. The location control has useSortBy in it and allows the user to provide their location and order results in distance from them order.

@mmorris8
Copy link
Author

By "filter that out in the search method" is this what you had in mind?

const algoliaClient = algoliasearch(appId, appKey);

const searchClient = {
  ...algoliaClient,
  search(requests) {
    const filtered = requests.filter( request => request.indexName !== 'root')
    const rootPresent = filtered.length === requests.length
    const query = algoliaClient.search(filtered)
    return query.then(response => {
      if (rootPresent) {
        response.results.push({
          index: 'root',
          hits: [],
          nbHits: 0,
          nbPages: 0,
          page: 0,
          processingTimeMS: 0
        })
      }
      return response
    })
  }
}

export default searchClient

Cause it sorta works, but the child indexes aren't getting their data and all the renders fail. Still sorting out why.

@Haroenv
Copy link
Contributor

Haroenv commented Nov 21, 2024

the fake response needs to be in the same position as in the requests, so the unshift instead of push (and really to be more correct you'd want to splice, keep track of the index etc, but in most cases it will be first)

@mmorris8
Copy link
Author

Since the response requests are in the same order other than the omission of the root a map is sufficient.

        response.results = requests.map(request => 
          request.indexName === 'root' ? {
            index: 'root',
            hits: [],
            nbHits: 0,
            nbPages: 0,
            page: 0,
            processingTimeMS: 0
          } : response.results.shift())
      }

Thank you for your time and support. This worked perfectly. I'm going to do a couple more refinements then start a discussion thread on this.

@Haroenv
Copy link
Contributor

Haroenv commented Nov 21, 2024

Hadn't considered this approach yet, but yes it should work. The order is always the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage Issues to be categorized by the team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants