Skip to content
This repository has been archived by the owner on Dec 30, 2022. It is now read-only.

Optional searchState cleanup when unmounting a connector #892

Closed
denkristoffer opened this issue Jan 23, 2018 · 22 comments
Closed

Optional searchState cleanup when unmounting a connector #892

denkristoffer opened this issue Jan 23, 2018 · 22 comments

Comments

@denkristoffer
Copy link
Contributor

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

@Haroenv
Copy link
Contributor

Haroenv commented Jan 23, 2018

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?

@denkristoffer
Copy link
Contributor Author

denkristoffer commented Jan 23, 2018

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 🙂

@Haroenv
Copy link
Contributor

Haroenv commented Jan 23, 2018

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

@denkristoffer
Copy link
Contributor Author

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?

@danhodkinson
Copy link

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.

@denkristoffer
Copy link
Contributor Author

@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 🤔

@danhodkinson
Copy link

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.

@Haroenv
Copy link
Contributor

Haroenv commented Jan 26, 2018

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!

@danhodkinson
Copy link

@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!

@Haroenv
Copy link
Contributor

Haroenv commented Jan 26, 2018

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)

@danhodkinson
Copy link

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
Copy link

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!

@samouss
Copy link
Collaborator

samouss commented Feb 1, 2018

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.

@danhodkinson
Copy link

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

@samouss
Copy link
Collaborator

samouss commented Feb 13, 2018

@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>
    );
  }
}

@denkristoffer
Copy link
Contributor Author

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.

@klaasman
Copy link

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
Copy link

klaasman commented May 24, 2019

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..

@ivawzh
Copy link

ivawzh commented Jul 3, 2019

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

@acornellier
Copy link

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

@cesarvarela
Copy link

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.

@dhayab
Copy link
Member

dhayab commented Dec 21, 2022

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

@dhayab dhayab closed this as completed Dec 21, 2022
@dhayab dhayab removed the dhaya label Dec 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests