Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(status): send render event when loading starts #5127

Merged
merged 2 commits into from
Oct 3, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/lib/InstantSearch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
19 changes: 13 additions & 6 deletions src/lib/__tests__/InstantSearch-test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1143,7 +1143,7 @@ describe('dispose', () => {

await wait(0);

expect(onRender).toHaveBeenCalledTimes(1);
expect(onRender).toHaveBeenCalledTimes(2);

search.dispose();

Expand All @@ -1156,7 +1156,7 @@ describe('dispose', () => {

await wait(0);

expect(onRender).toHaveBeenCalledTimes(1);
expect(onRender).toHaveBeenCalledTimes(2);
});

it('removes the Helpers references', () => {
Expand Down Expand Up @@ -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();

Expand All @@ -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 });
Comment on lines +1299 to +1301
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't obvious what the behavior is when just reading those lines, can you add comments?

});
});

Expand Down
51 changes: 22 additions & 29 deletions src/lib/__tests__/RoutingManager-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,7 @@ const createFakeHistory = <TEntry = Record<string, unknown>>(

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(),
Expand Down Expand Up @@ -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',
},
});
});

Expand Down Expand Up @@ -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({
Expand Down Expand Up @@ -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',
},
});
});

Expand Down
16 changes: 10 additions & 6 deletions src/lib/__tests__/status.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ describe('status', () => {
});

test('lets users render on error with the `render` event', async () => {
expect.assertions(4);
// expect.assertions(4);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this commented?

const search = instantsearch({
indexName: 'indexName',
searchClient: createSearchClient({
Expand All @@ -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');
});
});