From fe91dc7ff3ecaee735f92900f31416ab740490b7 Mon Sep 17 00:00:00 2001 From: Haroen Viaene Date: Fri, 30 Sep 2022 14:41:04 +0200 Subject: [PATCH] fix(status): send render event when loading starts Without this it's impossible to use `loading` for anything. I opted to only emit the event, as calling render on the widgets would require changes in all the tests. If we render (fully), you still don't get updated of `loading` initially because `render` only gets called when there are results: https://github.com/algolia/instantsearch.js/blob/4d43bd8292446101742a10d1da9f042f780d8d45/src/widgets/index/index.ts#L573-L575 To fix all of these inconsistencies, we should join `init` and `render` in a single function (always calling render, even if it's the first render, also when there's no results), but this would be a significant breaking change, so shouldn't be done lightly --- src/lib/InstantSearch.ts | 7 ++++ src/lib/__tests__/InstantSearch-test.tsx | 19 ++++++--- src/lib/__tests__/RoutingManager-test.ts | 51 ++++++++++-------------- src/lib/__tests__/status.test.ts | 16 +++++--- 4 files changed, 52 insertions(+), 41 deletions(-) diff --git a/src/lib/InstantSearch.ts b/src/lib/InstantSearch.ts index 1f04f11367..5607ae5821 100644 --- a/src/lib/InstantSearch.ts +++ b/src/lib/InstantSearch.ts @@ -478,6 +478,13 @@ See ${createDocumentationLink({ mainHelper.search = () => { this.status = 'loading'; + // @MAJOR: use scheduleRender here + // For now, widgets don't expect to be rendered at the start of `loading`, + // so it would be a breaking change to add an extra render. We don't have + // these guarantees about the render event, thus emitting it once more + // isn't a breaking change. + this.emit('render'); + // This solution allows us to keep the exact same API for the users but // under the hood, we have a different implementation. It should be // completely transparent for the rest of the codebase. Only this module diff --git a/src/lib/__tests__/InstantSearch-test.tsx b/src/lib/__tests__/InstantSearch-test.tsx index 9f2149d297..4d9e7abbac 100644 --- a/src/lib/__tests__/InstantSearch-test.tsx +++ b/src/lib/__tests__/InstantSearch-test.tsx @@ -1143,7 +1143,7 @@ describe('dispose', () => { await wait(0); - expect(onRender).toHaveBeenCalledTimes(1); + expect(onRender).toHaveBeenCalledTimes(2); search.dispose(); @@ -1156,7 +1156,7 @@ describe('dispose', () => { await wait(0); - expect(onRender).toHaveBeenCalledTimes(1); + expect(onRender).toHaveBeenCalledTimes(2); }); it('removes the Helpers references', () => { @@ -1273,12 +1273,12 @@ describe('scheduleRender', () => { expect(widget.render).toHaveBeenCalledTimes(1); }); - // eslint-disable-next-line jest/no-done-callback - it('emits a `render` event once the render is complete', (done) => { + it('emits a `render` event once the render is complete', async () => { const search = new InstantSearch({ indexName: 'indexName', searchClient: createSearchClient(), }); + const renderSpy = jest.fn(); const widget = createWidget(); @@ -1289,9 +1289,16 @@ describe('scheduleRender', () => { expect(widget.render).toHaveBeenCalledTimes(0); search.on('render', () => { - expect(widget.render).toHaveBeenCalledTimes(1); - done(); + renderSpy({ + widgetRenderTimes: castToJestMock(widget.render!).mock.calls.length, + }); }); + + await wait(0); + + expect(renderSpy).toHaveBeenCalledTimes(2); + expect(renderSpy).toHaveBeenNthCalledWith(1, { widgetRenderTimes: 0 }); + expect(renderSpy).toHaveBeenNthCalledWith(2, { widgetRenderTimes: 1 }); }); }); diff --git a/src/lib/__tests__/RoutingManager-test.ts b/src/lib/__tests__/RoutingManager-test.ts index 2366ac2576..f6982c2e1d 100644 --- a/src/lib/__tests__/RoutingManager-test.ts +++ b/src/lib/__tests__/RoutingManager-test.ts @@ -79,8 +79,7 @@ const createFakeHistory = >( describe('RoutingManager', () => { describe('within instantsearch', () => { - // eslint-disable-next-line jest/no-done-callback - test('should write in the router on searchParameters change', (done) => { + test('should write in the router on searchParameters change', async () => { const searchClient = createSearchClient(); const router = createFakeRouter({ write: jest.fn(), @@ -109,27 +108,25 @@ describe('RoutingManager', () => { search.start(); - search.once('render', async () => { - // initialization is done at this point - expect(widget.render).toHaveBeenCalledTimes(1); - expect(widget.getWidgetSearchParameters).toHaveBeenCalledTimes(1); + // initialization is done at this point + await wait(0); - await wait(0); + expect(widget.render).toHaveBeenCalledTimes(1); + expect(widget.getWidgetSearchParameters).toHaveBeenCalledTimes(1); - expect(router.write).toHaveBeenCalledTimes(0); + await wait(0); - search.mainIndex.getHelper()!.setQuery('q'); // routing write updates on change + expect(router.write).toHaveBeenCalledTimes(0); - await wait(0); + search.mainIndex.getHelper()!.setQuery('q'); // routing write updates on change - expect(router.write).toHaveBeenCalledTimes(1); - expect(router.write).toHaveBeenCalledWith({ - indexName: { - q: 'q', - }, - }); + await wait(0); - done(); + expect(router.write).toHaveBeenCalledTimes(1); + expect(router.write).toHaveBeenCalledWith({ + indexName: { + q: 'q', + }, }); }); @@ -184,8 +181,7 @@ describe('RoutingManager', () => { }); }); - // eslint-disable-next-line jest/no-done-callback - test('should apply state mapping on differences after searchFunction', (done) => { + test('should apply state mapping on differences after searchFunction', async () => { const searchClient = createSearchClient(); const router = createFakeRouter({ @@ -235,18 +231,15 @@ describe('RoutingManager', () => { search.start(); - search.once('render', () => { - // initialization is done at this point + await wait(0); + // initialization is done at this point - expect(search.mainIndex.getHelper()!.state.query).toEqual('test'); + expect(search.mainIndex.getHelper()!.state.query).toEqual('test'); - expect(router.write).toHaveBeenLastCalledWith({ - indexName: { - query: 'TEST', - }, - }); - - done(); + expect(router.write).toHaveBeenLastCalledWith({ + indexName: { + query: 'TEST', + }, }); }); diff --git a/src/lib/__tests__/status.test.ts b/src/lib/__tests__/status.test.ts index 252752b94d..4aa3797136 100644 --- a/src/lib/__tests__/status.test.ts +++ b/src/lib/__tests__/status.test.ts @@ -185,7 +185,7 @@ describe('status', () => { }); test('lets users render on error with the `render` event', async () => { - expect.assertions(4); + // expect.assertions(4); const search = instantsearch({ indexName: 'indexName', searchClient: createSearchClient({ @@ -210,10 +210,14 @@ describe('status', () => { search.start(); await wait(0); - expect(renderer).toHaveBeenCalledTimes(1); - const [{ status, error }] = renderer.mock.calls[0]; - expect(status).toBe('error'); - expect(error).toEqual(expect.any(Error)); - expect(error?.message).toEqual('Search error'); + expect(renderer).toHaveBeenCalledTimes(2); + const [[firstRender], [secondRender]] = renderer.mock.calls; + + expect(firstRender.status).toBe('loading'); + expect(firstRender.error).toEqual(undefined); + + expect(secondRender.status).toBe('error'); + expect(secondRender.error).toEqual(expect.any(Error)); + expect(secondRender.error?.message).toEqual('Search error'); }); });