Skip to content

Commit

Permalink
fix(error): directly emit error from events (#6472)
Browse files Browse the repository at this point in the history
* fix(error): directly emit error from events

In `@algolia/events`, errors are properly rethrown if they are thrown with `.emit('error', new Error("msg"))`, not if they are thrown with `.emit('error', { error: new Error('msg') })`. Therefore in InstantSearch and the helper, all errors are now properly thrown directly in the emit.

[FX-3187]

BREAKING CHANGE: if you have any `on('error'` handler that reads `event.error` from the callback, change that to just `error`

* !fixup
  • Loading branch information
Haroenv authored Dec 12, 2024
1 parent c8f143c commit 156c338
Show file tree
Hide file tree
Showing 11 changed files with 28 additions and 49 deletions.
4 changes: 2 additions & 2 deletions packages/algoliasearch-helper/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ declare namespace algoliasearchHelper {
event: 'result',
cb: (res: { results: SearchResults; state: SearchParameters }) => void
): this;
on(event: 'error', cb: (res: { error: Error }) => void): this;
on(event: 'error', cb: (res: Error) => void): this;
on(event: 'searchQueueEmpty', cb: () => void): this;

/**
Expand Down Expand Up @@ -338,7 +338,7 @@ declare namespace algoliasearchHelper {
};
}) => void
): this;
on(event: 'error', cb: (res: { error: Error }) => void): this;
on(event: 'error', cb: (res: Error) => void): this;

lastResults: SearchResults | null;
lastRecommendResults: RecommendResults | null;
Expand Down
16 changes: 4 additions & 12 deletions packages/algoliasearch-helper/src/algoliasearch.helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -1329,9 +1329,7 @@ AlgoliaSearchHelper.prototype._search = function (options) {
.catch(this._dispatchAlgoliaError.bind(this, queryId));
} catch (error) {
// If we reach this part, we're in an internal error state
this.emit('error', {
error: error,
});
this.emit('error', error);
}

return undefined;
Expand Down Expand Up @@ -1420,9 +1418,7 @@ AlgoliaSearchHelper.prototype._recommend = function () {
.catch(this._dispatchRecommendError.bind(this, queryId));
} catch (error) {
// If we reach this part, we're in an internal error state
this.emit('error', {
error: error,
});
this.emit('error', error);
}

return;
Expand Down Expand Up @@ -1579,9 +1575,7 @@ AlgoliaSearchHelper.prototype._dispatchAlgoliaError = function (
this._currentNbQueries -= queryId - this._lastQueryIdReceived;
this._lastQueryIdReceived = queryId;

this.emit('error', {
error: error,
});
this.emit('error', error);

if (this._currentNbQueries === 0) this.emit('searchQueueEmpty');
};
Expand All @@ -1599,9 +1593,7 @@ AlgoliaSearchHelper.prototype._dispatchRecommendError = function (
queryId - this._lastRecommendQueryIdReceived;
this._lastRecommendQueryIdReceived = queryId;

this.emit('error', {
error: error,
});
this.emit('error', error);

if (this._currentNbRecommendQueries === 0) this.emit('recommendQueueEmpty');
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ test('[INT][FILTERS] Using distinct should let me retrieve all facet without dis
facets: ['colors'],
});

helper.on('error', function (event) {
done.fail(event.error);
helper.on('error', function (error) {
done.fail(error);
});

helper.on('result', function (event) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ test('[INT][FILTERS] Should retrieve different values for multi facetted records

var calls = 0;

helper.on('error', function (event) {
done.fail(event.error);
helper.on('error', function (error) {
done.fail(error);
});

helper.on('result', function (event) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ test('[INT][NUMERICS][RAW-API]Test numeric operations on the helper and their re

var calls = 0;

helper.on('error', function (event) {
done.fail(event.error);
helper.on('error', function (error) {
done.fail(error);
});

helper.on('result', function (event) {
Expand Down Expand Up @@ -84,8 +84,8 @@ test('[INT][NUMERICS][MANAGED-API]Test numeric operations on the helper and thei

var calls = 0;

helper.on('error', function (event) {
done.fail(event.error);
helper.on('error', function (error) {
done.fail(error);
});

helper.on('result', function (event) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ test('[INT][SEARCHONCE] Should be able to search once with custom parameters wit
var state0 = helper.state;

var calls = 1;
helper.on('error', function (event) {
done.fail(event.error);
helper.on('error', function (error) {
done.fail(error);
});

helper.on('result', function (event) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ test('[INT][TAGS]Test tags operations on the helper and their results on the alg

var calls = 0;

helper.on('error', function (event) {
done.fail(event.error);
helper.on('error', function (error) {
done.fail(error);
});

helper.on('result', function (event) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -356,9 +356,7 @@ test('error event should be emitted once the request is complete with errors', f

return runAllMicroTasks().then(function () {
expect(errored).toHaveBeenCalledTimes(1);
expect(errored).toHaveBeenLastCalledWith({
error: expect.any(Error),
});
expect(errored).toHaveBeenLastCalledWith(expect.any(Error));
});
});

Expand All @@ -381,7 +379,5 @@ test('error event should be emitted if an error happens at request time', functi
helper.search();

expect(errored).toHaveBeenCalledTimes(1);
expect(errored).toHaveBeenLastCalledWith({
error: expect.any(Error),
});
expect(errored).toHaveBeenLastCalledWith(expect.any(Error));
});
Original file line number Diff line number Diff line change
Expand Up @@ -132,14 +132,11 @@ describe('errors', () => {

search.addWidgets([connectSearchBox(() => {})({})]);

expect.assertions(4);
expect.assertions(2);

search.on('error', (error) => {
expect(error).toBeInstanceOf(Error);
expect(error.message).toBe('test!');

expect(error.error).toBeInstanceOf(Error);
expect(error.error.message).toBe('test!');
});

search.start();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1108,9 +1108,9 @@ describe('start', () => {
search.addWidgets([virtualSearchBox({})]);
search.start();

search.on('error', (event) => {
search.on('error', (error) => {
expect(searchClient.search).toHaveBeenCalledTimes(1);
expect(event.error).toEqual(new Error('SERVER_ERROR'));
expect(error).toEqual(new Error('SERVER_ERROR'));
done();
});
});
Expand Down
16 changes: 5 additions & 11 deletions packages/instantsearch-core/src/instantsearch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -402,28 +402,22 @@ See documentation: ${createDocumentationLink({

// Only the "main" Helper emits the `error` event vs the one for `search`
// and `results` that are also emitted on the derived one.
mainHelper.on('error', ({ error }) => {
mainHelper.on('error', (error) => {
if (!(error instanceof Error)) {
// typescript lies here, error is in some cases { name: string, message: string }
const err = error as Record<string, any>;
error = Object.keys(err).reduce((acc, key) => {
this.error = Object.keys(err).reduce((acc, key) => {
(acc as any)[key] = err[key];
return acc;
}, new Error(err.message));
} else {
this.error = error;
}
// If an error is emitted, it is re-thrown by events. In previous versions
// we emitted {error}, which is thrown as:
// "Uncaught, unspecified \"error\" event. ([object Object])"
// To avoid breaking changes, we make the error available in both
// `error` and `error.error`
// @MAJOR emit only error
(error as any).error = error;
this.error = error;
this.status = 'error';
this.scheduleRender(false);

// This needs to execute last because it throws the error.
this.emit('error', error);
this.emit('error', this.error);
});

this.mainHelper = mainHelper;
Expand Down

0 comments on commit 156c338

Please sign in to comment.