algolia / vue-instantsearch

👀 Algolia components for building search UIs with Vue.js
https://www.algolia.com/doc/guides/building-search-ui/what-is-instantsearch/vue
MIT License
854 stars 157 forks source link

fix(SearchBox): currentRefinement slot prop is not in sync with custom input #1146

Closed ThomasKientz closed 1 year ago

ThomasKientz commented 1 year ago

fix https://github.com/algolia/vue-instantsearch/issues/1145

codesandbox-ci[bot] commented 1 year ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 4c83527962a84db32cbf1559b40bd47017bb706b:

Sandbox Source
vue-instantsearch-e-commerce Configuration
algolia reproduction issue Issue #1145
ThomasKientz commented 1 year ago

I came with a different approach, passing a method setIsFocused in the slot prop. More vue idiomatic imo and more explicit.

ThomasKientz commented 1 year ago

@Haroenv What do you think ?

Haroenv commented 1 year ago

Does this work for you? I've been trying to get to a working solution all day, but the input value seems to continue to "", because the setter of currentRefinement doesn't get called. It might be something on the setup, but I wonder if fundamentally trying to solve the synchronisation between those states might be wrong. Maybe we should document using closer to what we run in React InstantSearch

https://github.com/algolia/react-instantsearch/blob/master/packages/react-instantsearch-hooks-web/src/widgets/SearchBox.tsx#L75-L77

and putting this type of synchronisation code in a single pass rather than a computed value.

What do you think?

ThomasKientz commented 1 year ago

@Haroenv It does work with second approach using setIsFocus slot prop.

<ais-search-box>
  <template
    #default="{
      currentRefinement,
      isSearchStalled,
      refine,
      setIsFocus,
    }"
  >
    <SearchInput
      ref="searchInput"
      :value="currentRefinement"
      :loading="isSearchStalled"
      @focus="setIsFocus(true)"
      @blur="setIsFocus(false)"
      @input="refine($event)"
    />
  </template>
</ais-search-box>

I will have a look later on approach with react.

ThomasKientz commented 1 year ago

@Haroenv Have you tried with latest commit ? It does work with no issue. You can try it here : https://codesandbox.io/s/vue-instantsearch-e-commerce-0rgknz

Haroenv commented 1 year ago

@ThomasKientz, with this code the query actually never gets correctly updated: https://codesandbox.io/s/vue-instantsearch-e-commerce-forked-jpcjcj?file=/src/App.vue (as the local value gets used, but it never gets updated while it's focused)

https://user-images.githubusercontent.com/6270048/192267502-86491ba8-61a9-40bb-8757-976a0606eb2e.mov

While it's not ideal the default slot doesn't catch this race condition, I think it's better if you use createWidgetMixin for a custom input (I'll update the docs with that soon I think), as coordinating the focus state is too complex in Vue.

ThomasKientz commented 1 year ago

My bad, my PR doesn't work at all... Will look into createWidgetMixin.

Haroenv commented 1 year ago

No problem, I'll close the PR for now, thanks a lot and if you find another way to keep the inputs synced out of the box, that would be really cool (but unfortunately I've also looked for a while and can't find a sufficient solution)