react-instantsearch icon indicating copy to clipboard operation
react-instantsearch copied to clipboard

Optional searchState cleanup when unmounting a connector

Open denkristoffer opened this issue 7 years ago • 21 comments

Do you want to request a feature or report a bug?

Feature

Feature: What is your use case for such a feature?

I'm rendering a custom refinement list in an overlay with the connectRefinementList connector. I'd like the filters applied to stay on after the overlay is closed and unmounted. The connector currently runs the cleanUp method on unmount, which removes all refinements selected. I can create my own connector but it seems like a hassle to have to copy everything but the cleanup behaviour from the existing connectRefinementList.

Feature: What is your proposed API entry? The new option to add? What is the behavior?

I'm not sure what the best way to change the API would be here. Perhaps just a boolean prop stating whether or not to modify searchState on unmount?

What is the version you are using? Always use the latest one before opening a bug issue.

4.4.1

denkristoffer avatar Jan 23 '18 07:01 denkristoffer

The problem with not running the cleanup is that the search state is not longer deterministic. Do you think it would make sense to simply render nothing in your connector in a certain condition, but still render it, and thus also apply the "side effect" of applying the search?

Haroenv avatar Jan 23 '18 07:01 Haroenv

Hmm that's true, I hadn't considered that. My challenge is that I'm rendering a refinement list in different places depending on the device. Mobile users get the overlay which is rendered with a React portal, while other users see the refinement list elsewhere, outside the portal. If I render both at the same time, selecting a refinement will add it to the searchstate twice. I'll try to think about what can be done 🙂

denkristoffer avatar Jan 23 '18 07:01 denkristoffer

That's a great point though, portals are a unique use case. Can you make a small reproduction of what you have (and what you'd like to change)? A template is available here

Haroenv avatar Jan 23 '18 08:01 Haroenv

Can you make a small reproduction of what you have (and what you'd like to change)?

Something like this should show the issue. If you toggle the portal, set a refinement, and toggle the portal again, the filter will be disabled "universally", so to speak, even though there's still another refinement list rendered. When it comes to how it's best to solve it, I don't know yet 🙂

Maybe the best solution would be to keep track of the count of how many times the namespace has been used by mounting components, and only remove it once all the components have been unmounted?

denkristoffer avatar Jan 23 '18 12:01 denkristoffer

this is basically the same situation i've found myself in and i'm wondering which way to turn. I just have the one toggle portal for each facet. Once I unmount the portal, off goes my search.

Part of me is wondering whether this goes beyond how instantsearch wants to be used and it's best to simply start loading the search state in to my redux store to then update the defaultRefinements (potentially?). But it sure would be handy to be able to maintain the facets through a prop in the connector, so that the state was maintained.

danhodkinson avatar Jan 25 '18 20:01 danhodkinson

@danhodkinson Yeah, in your case my namespace count idea wouldn't help, but then we're back to the prop with the issue of deterministic search state 🤔

denkristoffer avatar Jan 26 '18 10:01 denkristoffer

indeed. Whilst i'm trying to find out if this is possible i'm going down the path of storing the search state in the redux store and syncing the main <InstantSearch/> with that. I can't think of a better way right now.

danhodkinson avatar Jan 26 '18 10:01 danhodkinson

For reference, we discussed this with the team, but have also not found a good solution yet. We keep this of course in mind, although right now every solution seems to have the same downsides (not being deterministic).

We also have a similar problem in #121.

Basically some way you can go around it now is, and it's definitely not an ideal solution, to create two InstantSearch instances and synchronise their state with onStateChange and the searchState prop. You should be able to do that without putting it in a redux store, or having it inside might also be possible.

Please keep us updated with what things you tried!

Haroenv avatar Jan 26 '18 11:01 Haroenv

@Haroenv

Basically some way you can go around it now is, and it's definitely not an ideal solution, to create two InstantSearch instances and synchronise their state with onStateChange and the searchState prop. You should be able to do that without putting it in a redux store, or having it inside might also be possible.

I think, if possible, keeping the searchState in redux is probably cleaner that having to have multiple <InstantSearch> wrappers everywhere you have a dialog box, I'm presuming that if you provide the main <InstantSearch> with the searchState prop then it should only listen to that?

The other part of me wonders if i'm having to go down the redux route should I be using React InstantSearch at all or should I just use the API reference. Decisions!

danhodkinson avatar Jan 26 '18 11:01 danhodkinson

It will still have a lot of advantages, specifically regarding widget design and accessibility and more complex features that require multiple requests. I'd use InstantSearch every time (and I develop both the API client and InstantSearch)

Haroenv avatar Jan 26 '18 12:01 Haroenv

revisiting this as i'm still having some issues. I may end up having to go down the route of creating my own connector due to the cleanUp function running.

It would be great to be able to take control over what happened with clean up running. CleanUpOnUnmount={false} would be very handy, especially for react native users.

But maybe there's more to this than i'm seeing.

danhodkinson avatar Feb 01 '18 12:02 danhodkinson

or potentially even a specific connectDialogRefinementList which was more tailored to dealing with refinements in modals/popovers.. anything that will unmount.

I'm attempting to build my own custom connector version of this.. it sure ain't easy!

danhodkinson avatar Feb 01 '18 15:02 danhodkinson

Thanks for the inputs @danhodkinson! This kind of API has already been discussed but abandoned because it doesn't fit well with the current implementation. We still not have the perfect solution, but we will move forward for find a proper API to handle this behaviour. We will try to find a proper workaround in the coming days, we will keep you updated.

samouss avatar Feb 01 '18 16:02 samouss

@samouss that's great news. Out of interest, for what reason doesn't it fit well with the current implementation?

danhodkinson avatar Feb 01 '18 17:02 danhodkinson

@denkristoffer @danhodkinson

We have a workaround that might work for your use cases. It's basically what has already been mention in the thread. You will need to keep track of the currentRefinements somewhere in your state (React, Redux, ....). For that you can listen to the searchStateChange event and to keep track of the current refined values. Then a VirtualRefinementList is always mount on the page and applies those refinements as a default value. The only drawbacks of this solution is that the App will trigger two requests when the Portal is open. The first one is triggered when an item of the <RefinementList> is clicked. The other one when we render the widgets with the defaultRefinement since it’s a new array on each searchStateChange the widget will trigger a new request.

Here is a working example with the proposed workaround on CodeSandbox. The deduplication part is only used by the <CurrentRefinements> widget, you can get rid of it if you don't use it.

Let us know how it goes with your use case 🙂

class App extends Component {
  state = {
    isOpen: false,
    brands: []
  };

  onSearchStateChange = searchState => {
    if (searchState.refinementList) {
      this.setState({
        brands: Array.isArray(searchState.refinementList.brand)
          ? searchState.refinementList.brand.slice()
          : []
      });
    }
  };

  toggle = () =>
    this.setState(prevState => ({
      isOpen: !prevState.isOpen
    }));

  render() {
    const { isOpen, brands } = this.state;

    return (
      <InstantSearch
        appId="latency"
        apiKey="6be0576ff61c053d5f9a3225e2a90f76"
        indexName="instant_search"
        onSearchStateChange={this.onSearchStateChange}
      >
        <VirtualRefinementList
          attributeName="brand"
          defaultRefinement={brands}
        />

        <SearchBox />

        <button onClick={this.toggle}>Toggle</button>

        {isOpen && (
          <RefinementList 
            attributeName="brand" 
            defaultRefinement={brands} 
          />
        )}

        <Hits />
      </InstantSearch>
    );
  }
}

samouss avatar Feb 13 '18 15:02 samouss

Thanks @samouss, this is a solid workaround, works well with a refinementList 👍I'm seeing a small issue when doing this with a range filter, but I'll make a separate issue for that.

denkristoffer avatar Feb 15 '18 13:02 denkristoffer

I've ran into the same issue and since a year has passed since the last activity, is there any change there has been made an improvement of the RefimentList api?

klaasman avatar May 24 '19 08:05 klaasman

Well, have a look at this solution over here: https://www.algolia.com/doc/guides/building-search-ui/going-further/native/react/?language=react#create-a-modal

It's written for react-native, but works perfectly with react too!


edit: I ran into another issue when the query is also depending on other queries, for example the SearchBox. The nested InstantSearch also needs all other query components, otherwise the facet results will not reflect the actual query itself.

So .. I'm afraid I still have not found the holy grail..

klaasman avatar May 24 '19 11:05 klaasman

This is still a huge difficulty with the lib. No satisfying solutions yet.

ivawzh avatar Jul 03 '19 18:07 ivawzh

Just voicing my concern, this is also making my life very difficult. No clean way around it.

acornellier avatar Mar 28 '21 13:03 acornellier

I just wasted 3 hours wondering why this was happening, using Modals + Portals from react-bootstrap.

A warning about this wouldn't hurt. This is not expected at all.

cesarvarela avatar Oct 29 '21 05:10 cesarvarela

Hi, we now provide documentation related to showing and hiding widgets.

dhayab avatar Dec 21 '22 13:12 dhayab