From fb30d433f668fdbfbdcb625372cb88f75c82a51a Mon Sep 17 00:00:00 2001 From: Haroen Viaene Date: Thu, 26 Oct 2023 11:17:50 +0200 Subject: [PATCH 1/3] refactor(core): move options diff to core Still open questions - how to do `insights` (does middleware need update option too?) (do we handle anything other than init props) - how to warn/error for routing/onStateChange (or do we handle it?) - test that it works properly of course - should some of the code in constructor be moved to update? --- .../instantsearch.js/src/lib/InstantSearch.ts | 92 ++++++++++++++++++- .../src/lib/useInstantSearchApi.ts | 66 ++----------- .../src/util/createInstantSearchComponent.js | 25 ++--- 3 files changed, 108 insertions(+), 75 deletions(-) diff --git a/packages/instantsearch.js/src/lib/InstantSearch.ts b/packages/instantsearch.js/src/lib/InstantSearch.ts index 634fbadf62..5a0728b051 100644 --- a/packages/instantsearch.js/src/lib/InstantSearch.ts +++ b/packages/instantsearch.js/src/lib/InstantSearch.ts @@ -178,6 +178,7 @@ export type InstantSearchStatus = 'idle' | 'loading' | 'stalled' | 'error'; export const INSTANTSEARCH_FUTURE_DEFAULTS: Required< InstantSearchOptions['future'] > = { preserveSharedStateOnUnmount: false }; +export const INSTANTSEARCH_STALLED_SEARCH_DEFAULTS = 200; /** * The actual implementation of the InstantSearch. This is @@ -250,7 +251,7 @@ Use \`InstantSearch.status === "stalled"\` instead.` routing = null, insights = undefined, searchFunction, - stalledSearchDelay = 200, + stalledSearchDelay = INSTANTSEARCH_STALLED_SEARCH_DEFAULTS, searchClient = null, insightsClient = null, onStateChange = null, @@ -840,6 +841,95 @@ See documentation: ${createDocumentationLink({ this.mainHelper.clearCache().search(); } + + /** + * Update the parameters passed to the InstantSearch constructor. + * @returns true if anything has been updated + */ + public update( + props: Partial> + ): boolean { + let hasUpdated = false; + if (this.indexName !== props.indexName) { + this.helper!.setIndex(props.indexName || '').search(); + hasUpdated = true; + } + + if (this.client !== props.searchClient) { + warning( + false, + // TODO: flavor-specific warning (search._flavor = 'vue') URL + 'The `searchClient` parameter changed, which may cause more search requests than necessary. If this is an unwanted behavior, please provide a stable reference.' + ); + + if (!props.searchClient) { + throw new Error(withUsage('The `searchClient` option is required.')); + } + + if (typeof props.searchClient.search !== 'function') { + throw new Error( + `The \`searchClient\` must implement a \`search\` method. + + See: https://www.algolia.com/doc/guides/building-search-ui/going-further/backend-search/in-depth/backend-instantsearch/js/` + ); + } + + // TODO: proper API for this too + // addAlgoliaAgents(props.searchClient, [ + // ...defaultUserAgents, + // serverContext && serverUserAgent, + // ]); + + this.mainHelper!.setClient(props.searchClient).search(); + hasUpdated = true; + } + + if (this.onStateChange !== props.onStateChange) { + this.onStateChange = props.onStateChange; + hasUpdated = true; + } + + if (this._searchFunction !== props.searchFunction) { + // Updating the `searchFunction` to `undefined` is not supported by + // InstantSearch.js, so it will throw an error. + // This is a fair behavior until we add an update API in InstantSearch.js. + // TODO: unset if next searchFunction is undefined, maybe set to a noop? + this._searchFunction = + props.searchFunction ?? + ((helper) => { + helper.search(); + }); + hasUpdated = true; + } + + if (this._stalledSearchDelay !== props.stalledSearchDelay) { + this._stalledSearchDelay = + props.stalledSearchDelay ?? INSTANTSEARCH_STALLED_SEARCH_DEFAULTS; + hasUpdated = true; + } + + // TODO: move dequal to a shared package? + // if (!dequal(this.future, props.future)) { + // this.future = { + // ...INSTANTSEARCH_FUTURE_DEFAULTS, + // ...props.future, + // }; + // hasUpdated = true; + // } + + // TODO: insights option + // TODO: validate other options (onStateChange, routing, give a warning for all non-updated values?) + + // Updating the `routing` prop is not supported because InstantSearch.js + // doesn't let us change it. This might not be a problem though, because `routing` + // shouldn't need to be dynamic. + // If we find scenarios where `routing` needs to change, we can always expose + // it privately on the InstantSearch instance. Another way would be to + // manually inject the routing middleware in this library, and not rely + // on the provided `routing` prop. + + return hasUpdated; + } } export default InstantSearch; diff --git a/packages/react-instantsearch-core/src/lib/useInstantSearchApi.ts b/packages/react-instantsearch-core/src/lib/useInstantSearchApi.ts index 1ea5c75bad..76c885f0a9 100644 --- a/packages/react-instantsearch-core/src/lib/useInstantSearchApi.ts +++ b/packages/react-instantsearch-core/src/lib/useInstantSearchApi.ts @@ -1,12 +1,9 @@ -import InstantSearch, { - INSTANTSEARCH_FUTURE_DEFAULTS, -} from 'instantsearch.js/es/lib/InstantSearch'; +import InstantSearch from 'instantsearch.js/es/lib/InstantSearch'; import { useCallback, useRef, version as ReactVersion } from 'react'; import { useSyncExternalStore } from 'use-sync-external-store/shim'; import version from '../version'; -import { dequal } from './dequal'; import { useForceUpdate } from './useForceUpdate'; import { useInstantSearchServerContext } from './useInstantSearchServerContext'; import { useInstantSearchSSRContext } from './useInstantSearchSSRContext'; @@ -136,62 +133,13 @@ export function useInstantSearchApi( { const search = searchRef.current; - const prevProps = prevPropsRef.current; - - if (prevProps.indexName !== props.indexName) { - search.helper!.setIndex(props.indexName || '').search(); - prevPropsRef.current = props; - } - - if (prevProps.searchClient !== props.searchClient) { - warn( - false, - 'The `searchClient` prop of `` changed between renders, which may cause more search requests than necessary. If this is an unwanted behavior, please provide a stable reference: https://www.algolia.com/doc/api-reference/widgets/instantsearch/react/#widget-param-searchclient' - ); - - addAlgoliaAgents(props.searchClient, [ - ...defaultUserAgents, - serverContext && serverUserAgent, - ]); - search.mainHelper!.setClient(props.searchClient).search(); - prevPropsRef.current = props; - } - - if (prevProps.onStateChange !== props.onStateChange) { - search.onStateChange = props.onStateChange; - prevPropsRef.current = props; - } - - if (prevProps.searchFunction !== props.searchFunction) { - // Updating the `searchFunction` to `undefined` is not supported by - // InstantSearch.js, so it will throw an error. - // This is a fair behavior until we add an update API in InstantSearch.js. - search._searchFunction = props.searchFunction; - prevPropsRef.current = props; + // TODO: is this still needed? (was it even ever needed, we could have read the private InstantSearch values) + // const prevProps = prevPropsRef.current; + const nextProps = props; + const updated = search.update(nextProps); + if (updated) { + prevPropsRef.current = nextProps; } - - if (prevProps.stalledSearchDelay !== props.stalledSearchDelay) { - // The default `stalledSearchDelay` in InstantSearch.js is 200ms. - // We need to reset it when it's undefined to get back to the original value. - search._stalledSearchDelay = props.stalledSearchDelay ?? 200; - prevPropsRef.current = props; - } - - if (!dequal(prevProps.future, props.future)) { - search.future = { - ...INSTANTSEARCH_FUTURE_DEFAULTS, - ...props.future, - }; - prevPropsRef.current = props; - } - - // Updating the `routing` prop is not supported because InstantSearch.js - // doesn't let us change it. This might not be a problem though, because `routing` - // shouldn't need to be dynamic. - // If we find scenarios where `routing` needs to change, we can always expose - // it privately on the InstantSearch instance. Another way would be to - // manually inject the routing middleware in this library, and not rely - // on the provided `routing` prop. } const cleanupTimerRef = useRef | null>(null); diff --git a/packages/vue-instantsearch/src/util/createInstantSearchComponent.js b/packages/vue-instantsearch/src/util/createInstantSearchComponent.js index c172ceb326..eacb9f029c 100644 --- a/packages/vue-instantsearch/src/util/createInstantSearchComponent.js +++ b/packages/vue-instantsearch/src/util/createInstantSearchComponent.js @@ -18,21 +18,18 @@ export const createInstantSearchComponent = (component) => }, watch: { searchClient(searchClient) { - warn( - false, - 'The `search-client` prop of `` changed between renders, which may cause more search requests than necessary. If this is an unwanted behavior, please provide a stable reference: https://www.algolia.com/doc/api-reference/widgets/instantsearch/vue/#widget-param-search-client' - ); - - this.instantSearchInstance.helper.setClient(searchClient).search(); + this.instantSearchInstance.update({ searchClient }); }, indexName(indexName) { - this.instantSearchInstance.helper.setIndex(indexName || '').search(); + this.instantSearchInstance.update({ indexName }); }, stalledSearchDelay(stalledSearchDelay) { - // private InstantSearch.js API: - this.instantSearchInstance._stalledSearchDelay = stalledSearchDelay; + this.instantSearchInstance.update({ stalledSearchDelay }); }, routing() { + // TODO: in React this is ignored, in Vue an error + // How do we get a consistent behaviour? + // Nobody ever opened these issues, implying error is right behaviour? throw new Error( 'routing configuration can not be changed dynamically at this point.' + '\n\n' + @@ -47,9 +44,10 @@ export const createInstantSearchComponent = (component) => ); }, searchFunction(searchFunction) { - // private InstantSearch.js API: - this.instantSearchInstance._searchFunction = searchFunction; + this.instantSearchInstance.update({ searchFunction }); }, + // TODO: insights + // TODO: should this be an InstantSearch API? maybe not middlewares: { immediate: true, handler(next, prev) { @@ -67,10 +65,7 @@ export const createInstantSearchComponent = (component) => }, }, future(future) { - this.instantSearchInstance.future = Object.assign( - INSTANTSEARCH_FUTURE_DEFAULTS, - future - ); + this.instantSearchInstance.update({ future }); }, }, created() { From 196b63e1f684d53062ac004eeb9cd95c9fb463e6 Mon Sep 17 00:00:00 2001 From: Haroen Viaene Date: Thu, 26 Oct 2023 11:21:01 +0200 Subject: [PATCH 2/3] remove hasUpdated --- packages/instantsearch.js/src/lib/InstantSearch.ts | 12 +----------- .../src/lib/useInstantSearchApi.ts | 12 +----------- 2 files changed, 2 insertions(+), 22 deletions(-) diff --git a/packages/instantsearch.js/src/lib/InstantSearch.ts b/packages/instantsearch.js/src/lib/InstantSearch.ts index 5a0728b051..a275cd46d7 100644 --- a/packages/instantsearch.js/src/lib/InstantSearch.ts +++ b/packages/instantsearch.js/src/lib/InstantSearch.ts @@ -844,15 +844,12 @@ See documentation: ${createDocumentationLink({ /** * Update the parameters passed to the InstantSearch constructor. - * @returns true if anything has been updated */ public update( props: Partial> - ): boolean { - let hasUpdated = false; + ): void { if (this.indexName !== props.indexName) { this.helper!.setIndex(props.indexName || '').search(); - hasUpdated = true; } if (this.client !== props.searchClient) { @@ -881,12 +878,10 @@ See documentation: ${createDocumentationLink({ // ]); this.mainHelper!.setClient(props.searchClient).search(); - hasUpdated = true; } if (this.onStateChange !== props.onStateChange) { this.onStateChange = props.onStateChange; - hasUpdated = true; } if (this._searchFunction !== props.searchFunction) { @@ -899,13 +894,11 @@ See documentation: ${createDocumentationLink({ ((helper) => { helper.search(); }); - hasUpdated = true; } if (this._stalledSearchDelay !== props.stalledSearchDelay) { this._stalledSearchDelay = props.stalledSearchDelay ?? INSTANTSEARCH_STALLED_SEARCH_DEFAULTS; - hasUpdated = true; } // TODO: move dequal to a shared package? @@ -914,7 +907,6 @@ See documentation: ${createDocumentationLink({ // ...INSTANTSEARCH_FUTURE_DEFAULTS, // ...props.future, // }; - // hasUpdated = true; // } // TODO: insights option @@ -927,8 +919,6 @@ See documentation: ${createDocumentationLink({ // it privately on the InstantSearch instance. Another way would be to // manually inject the routing middleware in this library, and not rely // on the provided `routing` prop. - - return hasUpdated; } } diff --git a/packages/react-instantsearch-core/src/lib/useInstantSearchApi.ts b/packages/react-instantsearch-core/src/lib/useInstantSearchApi.ts index 76c885f0a9..2bfdf3e513 100644 --- a/packages/react-instantsearch-core/src/lib/useInstantSearchApi.ts +++ b/packages/react-instantsearch-core/src/lib/useInstantSearchApi.ts @@ -59,7 +59,6 @@ export function useInstantSearchApi( const serverState = useInstantSearchSSRContext(); const waitingForResultsRef = useRSCContext(); const initialResults = serverState?.initialResults; - const prevPropsRef = useRef(props); const shouldRenderAtOnce = serverContext || initialResults || waitingForResultsRef; @@ -131,16 +130,7 @@ export function useInstantSearchApi( searchRef.current = search; } - { - const search = searchRef.current; - // TODO: is this still needed? (was it even ever needed, we could have read the private InstantSearch values) - // const prevProps = prevPropsRef.current; - const nextProps = props; - const updated = search.update(nextProps); - if (updated) { - prevPropsRef.current = nextProps; - } - } + searchRef.current.update(props); const cleanupTimerRef = useRef | null>(null); const store = useSyncExternalStore>( From d7186ad1754ed5c38d50e88ab07c7ac076d798b2 Mon Sep 17 00:00:00 2001 From: Haroen Viaene Date: Thu, 26 Oct 2023 12:18:55 +0200 Subject: [PATCH 3/3] indication --- .../instantsearch.js/src/lib/InstantSearch.ts | 56 ++++++++++++------- 1 file changed, 36 insertions(+), 20 deletions(-) diff --git a/packages/instantsearch.js/src/lib/InstantSearch.ts b/packages/instantsearch.js/src/lib/InstantSearch.ts index a275cd46d7..bb23618fbe 100644 --- a/packages/instantsearch.js/src/lib/InstantSearch.ts +++ b/packages/instantsearch.js/src/lib/InstantSearch.ts @@ -365,8 +365,9 @@ See documentation: ${createDocumentationLink({ // This is the default Insights middleware, // added when `insights` is set to true by the user. // Any user-provided middleware will be added later and override this one. - if (insights) { - const insightsOptions = typeof insights === 'boolean' ? {} : insights; + if (this._insights) { + const insightsOptions = + typeof this._insights === 'boolean' ? {} : this._insights; insightsOptions.$$internal = true; this.use(createInsightsMiddleware(insightsOptions)); } @@ -846,24 +847,24 @@ See documentation: ${createDocumentationLink({ * Update the parameters passed to the InstantSearch constructor. */ public update( - props: Partial> + options: Partial> ): void { - if (this.indexName !== props.indexName) { - this.helper!.setIndex(props.indexName || '').search(); + if (this.indexName !== options.indexName) { + this.helper!.setIndex(options.indexName || '').search(); } - if (this.client !== props.searchClient) { + if (this.client !== options.searchClient) { warning( false, // TODO: flavor-specific warning (search._flavor = 'vue') URL 'The `searchClient` parameter changed, which may cause more search requests than necessary. If this is an unwanted behavior, please provide a stable reference.' ); - if (!props.searchClient) { + if (!options.searchClient) { throw new Error(withUsage('The `searchClient` option is required.')); } - if (typeof props.searchClient.search !== 'function') { + if (typeof options.searchClient.search !== 'function') { throw new Error( `The \`searchClient\` must implement a \`search\` method. @@ -872,33 +873,30 @@ See documentation: ${createDocumentationLink({ } // TODO: proper API for this too + // InstantSearch._algoliaAgents (or constructor) // addAlgoliaAgents(props.searchClient, [ // ...defaultUserAgents, // serverContext && serverUserAgent, // ]); - this.mainHelper!.setClient(props.searchClient).search(); + this.mainHelper!.setClient(options.searchClient).search(); } - if (this.onStateChange !== props.onStateChange) { - this.onStateChange = props.onStateChange; + if (this.onStateChange !== options.onStateChange) { + this.onStateChange = options.onStateChange; } - if (this._searchFunction !== props.searchFunction) { - // Updating the `searchFunction` to `undefined` is not supported by - // InstantSearch.js, so it will throw an error. - // This is a fair behavior until we add an update API in InstantSearch.js. - // TODO: unset if next searchFunction is undefined, maybe set to a noop? + if (this._searchFunction !== options.searchFunction) { this._searchFunction = - props.searchFunction ?? + options.searchFunction ?? ((helper) => { helper.search(); }); } - if (this._stalledSearchDelay !== props.stalledSearchDelay) { + if (this._stalledSearchDelay !== options.stalledSearchDelay) { this._stalledSearchDelay = - props.stalledSearchDelay ?? INSTANTSEARCH_STALLED_SEARCH_DEFAULTS; + options.stalledSearchDelay ?? INSTANTSEARCH_STALLED_SEARCH_DEFAULTS; } // TODO: move dequal to a shared package? @@ -909,7 +907,25 @@ See documentation: ${createDocumentationLink({ // }; // } - // TODO: insights option + // TODO: implement update in middleware + // TODO: can this be simplified? + // if (!dequal(this._insights, options.insights)) { + // this._insights = options.insights; + // const existing = this.middleware.find( + // (m) => m.instance.$$type === 'ais.insights' && m.instance.$$internal + // ); + // if (options.insights && existing) { + // // TODO: update existing middleware somehow (or maybe remount it) + // } else if (options.insights && !existing) { + // const insightsOptions = + // typeof options.insights === 'boolean' ? {} : options.insights; + // insightsOptions.$$internal = true; + // this.use(createInsightsMiddleware(insightsOptions)); + // } else if (!options.insights && existing) { + // this.unuse(existing.creator); + // } + // } + // TODO: validate other options (onStateChange, routing, give a warning for all non-updated values?) // Updating the `routing` prop is not supported because InstantSearch.js