From a342cd1e84c03f9b660a533b327a2f1c12682121 Mon Sep 17 00:00:00 2001 From: Chris Date: Mon, 9 Dec 2024 14:07:54 -0800 Subject: [PATCH] fix(controller): bugfix for controller store loading / loaded states --- .../AutocompleteController.test.ts | 21 ++++++--- .../Autocomplete/AutocompleteController.ts | 46 ++++++++----------- .../src/Finder/FinderController.test.ts | 28 +++++++++++ .../src/Finder/FinderController.ts | 30 ++++++------ .../RecommendationController.test.ts | 28 +++++++++++ .../RecommendationController.ts | 26 ++++------- .../src/Search/SearchController.test.ts | 17 ++++++- .../src/Search/SearchController.ts | 33 +++++-------- 8 files changed, 140 insertions(+), 89 deletions(-) diff --git a/packages/snap-controller/src/Autocomplete/AutocompleteController.test.ts b/packages/snap-controller/src/Autocomplete/AutocompleteController.test.ts index da8c4be44..89b6bb3b3 100644 --- a/packages/snap-controller/src/Autocomplete/AutocompleteController.test.ts +++ b/packages/snap-controller/src/Autocomplete/AutocompleteController.test.ts @@ -158,20 +158,27 @@ describe('Autocomplete Controller', () => { tracker: new Tracker(globals), }); - controller.init(); + // calling init to ensure event timings line up for asserting loading and loaded states + await controller.init(); const query = 'wh'; controller.urlManager = controller.urlManager.reset().set('query', query); expect(controller.urlManager.state.query).toBe(query); (controller.client as MockClient).mockData.updateConfig({ autocomplete: 'autocomplete.query.wh' }); - await controller.search(); + const searchPromise = controller.search(); - await waitFor(() => { - expect(controller.store.results.length).toBeGreaterThan(0); - expect(controller.store.results.length).toBe(acConfig.globals!.pagination!.pageSize); - expect(controller.store.terms.length).toBe(acConfig.globals!.suggestions!.count); - }); + expect(controller.store.loaded).toBe(false); + expect(controller.store.loading).toBe(true); + + await searchPromise; + + expect(controller.store.loaded).toBe(true); + expect(controller.store.loading).toBe(false); + + expect(controller.store.results.length).toBeGreaterThan(0); + expect(controller.store.results.length).toBe(acConfig.globals!.pagination!.pageSize); + expect(controller.store.terms.length).toBe(acConfig.globals!.suggestions!.count); }); it('has no results if query is blank', async () => { diff --git a/packages/snap-controller/src/Autocomplete/AutocompleteController.ts b/packages/snap-controller/src/Autocomplete/AutocompleteController.ts index f724dc847..5290d6737 100644 --- a/packages/snap-controller/src/Autocomplete/AutocompleteController.ts +++ b/packages/snap-controller/src/Autocomplete/AutocompleteController.ts @@ -6,7 +6,7 @@ import { getSearchParams } from '../utils/getParams'; import { ControllerTypes } from '../types'; import { AutocompleteStore } from '@searchspring/snap-store-mobx'; -import type { AutocompleteControllerConfig, BeforeSearchObj, AfterSearchObj, AfterStoreObj, ControllerServices, ContextVariables } from '../types'; +import type { AutocompleteControllerConfig, AfterSearchObj, AfterStoreObj, ControllerServices, ContextVariables } from '../types'; import type { Next } from '@searchspring/snap-event-manager'; import type { AutocompleteRequestModel } from '@searchspring/snapi-types'; @@ -80,31 +80,16 @@ export class AutocompleteController extends AbstractController { key: `ss-controller-${this.config.id}`, }); - // add 'beforeSearch' middleware - this.eventManager.on('beforeSearch', async (ac: BeforeSearchObj, next: Next): Promise => { - ac.controller.store.loading = true; - - await next(); - }); - // add 'afterSearch' middleware this.eventManager.on('afterSearch', async (ac: AfterSearchObj, next: Next): Promise => { await next(); // cancel search if no input or query doesn't match current urlState if (ac.response.autocomplete.query != ac.controller.urlManager.state.query) { - ac.controller.store.loading = false; return false; } }); - // add 'afterStore' middleware - this.eventManager.on('afterStore', async (ac: AfterStoreObj, next: Next): Promise => { - await next(); - - ac.controller.store.loading = false; - }); - this.eventManager.on('beforeSubmit', async (ac: AfterStoreObj, next: Next): Promise => { await next(); @@ -540,19 +525,25 @@ export class AutocompleteController extends AbstractController { }; search = async (): Promise => { - // if urlManager has no query, there will be no need to get params and no query - if (!this.urlManager.state.query) { - return; - } + try { + if (!this.initialized) { + await this.init(); + } - const params = this.params; + // if urlManager has no query, there will be no need to get params and no query + if (!this.urlManager.state.query) { + return; + } - // if params have no query do not search - if (!params?.search?.query?.string) { - return; - } + const params = this.params; + + // if params have no query do not search + if (!params?.search?.query?.string) { + return; + } + + this.store.loading = true; - try { try { await this.eventManager.fire('beforeSearch', { controller: this, @@ -667,8 +658,9 @@ export class AutocompleteController extends AbstractController { this.log.error(err); this.handleError(err); } - this.store.loading = false; } + } finally { + this.store.loading = false; } }; } diff --git a/packages/snap-controller/src/Finder/FinderController.test.ts b/packages/snap-controller/src/Finder/FinderController.test.ts index dcf3e0c36..3aac4fae4 100644 --- a/packages/snap-controller/src/Finder/FinderController.test.ts +++ b/packages/snap-controller/src/Finder/FinderController.test.ts @@ -152,6 +152,34 @@ describe('Finder Controller', () => { }); }); + it(`sets proper loading states`, async function () { + const controller = new FinderController(config, { + client: new MockClient(globals, {}), + store: new FinderStore(config, services), + urlManager, + eventManager: new EventManager(), + profiler: new Profiler(), + logger: new Logger(), + tracker: new Tracker(globals), + }); + + // calling init to ensure event timings line up for asserting loading and loaded states + await controller.init(); + + expect(controller.store.loaded).toBe(false); + expect(controller.store.loading).toBe(false); + + const searchPromise = controller.search(); + + expect(controller.store.loaded).toBe(false); + expect(controller.store.loading).toBe(true); + + await searchPromise; + + expect(controller.store.loaded).toBe(true); + expect(controller.store.loading).toBe(false); + }); + it(`sets root URL params`, async () => { config.url = '/search/accessories'; const controller = new FinderController(config, { diff --git a/packages/snap-controller/src/Finder/FinderController.ts b/packages/snap-controller/src/Finder/FinderController.ts index d663ff603..395389809 100644 --- a/packages/snap-controller/src/Finder/FinderController.ts +++ b/packages/snap-controller/src/Finder/FinderController.ts @@ -7,7 +7,7 @@ import { getSearchParams } from '../utils/getParams'; import { ControllerTypes } from '../types'; import type { FinderStore } from '@searchspring/snap-store-mobx'; import type { Next } from '@searchspring/snap-event-manager'; -import type { FinderControllerConfig, BeforeSearchObj, AfterSearchObj, ControllerServices, ContextVariables } from '../types'; +import type { FinderControllerConfig, AfterSearchObj, ControllerServices, ContextVariables } from '../types'; const defaultConfig: FinderControllerConfig = { id: 'finder', @@ -50,13 +50,7 @@ export class FinderController extends AbstractController { }); } - this.eventManager.on('beforeSearch', async (finder: BeforeSearchObj, next: Next): Promise => { - finder.controller.store.loading = true; - - await next(); - }); - - // TODO: move this to afterStore + // TODO: remove this aftersearch when store interface changes this.eventManager.on('afterSearch', async (finder: AfterSearchObj, next: Next): Promise => { await next(); @@ -129,17 +123,18 @@ export class FinderController extends AbstractController { }; search = async (): Promise => { - if (!this.initialized) { - await this.init(); - } + try { + if (!this.initialized) { + await this.init(); + } - if (this.store.state.persisted) { - return; - } + if (this.store.state.persisted) { + return; + } - const params = this.params; + const params = this.params; - try { + this.store.loading = true; try { await this.eventManager.fire('beforeSearch', { controller: this, @@ -255,8 +250,9 @@ export class FinderController extends AbstractController { this.log.error(err); this.handleError(err); } - this.store.loading = false; } + } finally { + this.store.loading = false; } }; } diff --git a/packages/snap-controller/src/Recommendation/RecommendationController.test.ts b/packages/snap-controller/src/Recommendation/RecommendationController.test.ts index fdf5c56c7..0323dab0b 100644 --- a/packages/snap-controller/src/Recommendation/RecommendationController.test.ts +++ b/packages/snap-controller/src/Recommendation/RecommendationController.test.ts @@ -118,6 +118,34 @@ describe('Recommendation Controller', () => { }); }); + it(`sets proper loading states`, async function () { + const controller = new RecommendationController(recommendConfig, { + client: new MockClient(globals, {}), + store: new RecommendationStore(recommendConfig, services), + urlManager, + eventManager: new EventManager(), + profiler: new Profiler(), + logger: new Logger(), + tracker: new Tracker(globals), + }); + + // calling init to ensure event timings line up for asserting loading and loaded states + await controller.init(); + + expect(controller.store.loaded).toBe(false); + expect(controller.store.loading).toBe(false); + + const searchPromise = controller.search(); + + expect(controller.store.loaded).toBe(false); + expect(controller.store.loading).toBe(true); + + await searchPromise; + + expect(controller.store.loaded).toBe(true); + expect(controller.store.loading).toBe(false); + }); + it(`tests searchOnPageShow triggers search on persisted pageshow event `, async function () { const controller = new RecommendationController(recommendConfig, { client: new MockClient(globals, {}), diff --git a/packages/snap-controller/src/Recommendation/RecommendationController.ts b/packages/snap-controller/src/Recommendation/RecommendationController.ts index 31155842b..c5ef60d9e 100644 --- a/packages/snap-controller/src/Recommendation/RecommendationController.ts +++ b/packages/snap-controller/src/Recommendation/RecommendationController.ts @@ -9,7 +9,7 @@ import type { ProductViewEvent } from '@searchspring/snap-tracker'; import type { RecommendationStore } from '@searchspring/snap-store-mobx'; import type { Next } from '@searchspring/snap-event-manager'; import type { RecommendRequestModel } from '@searchspring/snap-client'; -import type { RecommendationControllerConfig, BeforeSearchObj, AfterStoreObj, ControllerServices, ContextVariables } from '../types'; +import type { RecommendationControllerConfig, AfterStoreObj, ControllerServices, ContextVariables } from '../types'; type RecommendationTrackMethods = { product: { @@ -75,13 +75,6 @@ export class RecommendationController extends AbstractController { this.config = deepmerge(defaultConfig, this.config); this.store.setConfig(this.config); - // add 'beforeSearch' middleware - this.eventManager.on('beforeSearch', async (recommend: BeforeSearchObj, next: Next): Promise => { - recommend.controller.store.loading = true; - - await next(); - }); - // add 'afterStore' middleware this.eventManager.on('afterStore', async (recommend: AfterStoreObj, next: Next): Promise => { await next(); @@ -98,8 +91,6 @@ export class RecommendationController extends AbstractController { this.track.product.removedFromBundle(item); }); }); - - recommend.controller.store.loading = false; }); // attach config plugins and event middleware @@ -434,13 +425,15 @@ export class RecommendationController extends AbstractController { } search = async (): Promise => { - if (!this.initialized) { - await this.init(); - } + try { + if (!this.initialized) { + await this.init(); + } - const params = this.params; + const params = this.params; + + this.store.loading = true; - try { try { await this.eventManager.fire('beforeSearch', { controller: this, @@ -549,8 +542,9 @@ export class RecommendationController extends AbstractController { this.log.error(err); this.handleError(err); } - this.store.loading = false; } + } finally { + this.store.loading = false; } }; } diff --git a/packages/snap-controller/src/Search/SearchController.test.ts b/packages/snap-controller/src/Search/SearchController.test.ts index bdefc751d..3bdce1e66 100644 --- a/packages/snap-controller/src/Search/SearchController.test.ts +++ b/packages/snap-controller/src/Search/SearchController.test.ts @@ -49,6 +49,9 @@ describe('Search Controller', () => { expect(initfn).not.toHaveBeenCalled(); controller.init(); + expect(controller.store.loaded).toBe(false); + expect(controller.store.loading).toBe(false); + expect(initfn).toHaveBeenCalled(); expect(controller instanceof SearchController).toBeTruthy(); @@ -63,7 +66,19 @@ describe('Search Controller', () => { expect(controller.store.results.length).toBe(0); expect(searchfn).not.toHaveBeenCalled(); - await controller.search(); + + // calling init to ensure event timings line up for asserting loading and loaded states + await controller.init(); + + const searchPromise = controller.search(); + + expect(controller.store.loaded).toBe(false); + expect(controller.store.loading).toBe(true); + + await searchPromise; + expect(controller.store.loaded).toBe(true); + expect(controller.store.loading).toBe(false); + expect(searchfn).toHaveBeenCalled(); expect(controller.store.results.length).toBeGreaterThan(0); diff --git a/packages/snap-controller/src/Search/SearchController.ts b/packages/snap-controller/src/Search/SearchController.ts index 896718b4b..f0fb7a9c7 100644 --- a/packages/snap-controller/src/Search/SearchController.ts +++ b/packages/snap-controller/src/Search/SearchController.ts @@ -10,7 +10,6 @@ import type { BeaconEvent } from '@searchspring/snap-tracker'; import type { SearchStore } from '@searchspring/snap-store-mobx'; import type { SearchControllerConfig, - BeforeSearchObj, AfterSearchObj, AfterStoreObj, ControllerServices, @@ -82,13 +81,6 @@ export class SearchController extends AbstractController { // set last params to undefined for compare in search this.storage.set('lastStringyParams', undefined); - // add 'beforeSearch' middleware - this.eventManager.on('beforeSearch', async (search: BeforeSearchObj, next: Next): Promise => { - search.controller.store.loading = true; - - await next(); - }); - // add 'afterSearch' middleware this.eventManager.on('afterSearch', async (search: AfterSearchObj, next: Next): Promise => { const config = search.controller.config as SearchControllerConfig; @@ -131,8 +123,6 @@ export class SearchController extends AbstractController { } await this.eventManager.fire('restorePosition', { controller: this, element: elementPosition }); - - search.controller.store.loading = false; }); // restore position @@ -321,18 +311,18 @@ export class SearchController extends AbstractController { } search = async (): Promise => { - if (!this.initialized) { - await this.init(); - } - - const params = this.params; + try { + if (!this.initialized) { + await this.init(); + } + const params = this.params; - if (this.params.search?.query?.string && this.params.search?.query?.string.length) { - // save it to the history store - this.store.history.save(this.params.search.query.string); - } + if (this.params.search?.query?.string && this.params.search?.query?.string.length) { + // save it to the history store + this.store.history.save(this.params.search.query.string); + } + this.store.loading = true; - try { try { await this.eventManager.fire('beforeSearch', { controller: this, @@ -527,8 +517,9 @@ export class SearchController extends AbstractController { this.log.error(err); this.handleError(err); } - this.store.loading = false; } + } finally { + this.store.loading = false; } }; }