Improve errors #1656

flevi29 · 2024-05-22T12:30:19Z

Pull Request

Related issues

What does this PR do?

This PR aims to improve errors, so that they can contain all the necessary information, and more.

Prevent browser test jsdom from replacing fetch and AbortController for consistency with node tests, replacing previous solution where we removed the builtin fetch from node tests
Remove "abort-controller" package, it was only used in tests and now AbortController is always available
Rename MeiliSearchCommunicationError to MeiliSearchRequestError, as this error might not be entirely related to communication, but rather the request itself
Make errors use cause, preserving the original error, simplifying things, taking advantage of modern browsers and runtimes actually printing this property
Remove the use of Object.setPrototypeOf in errors, this is not needed in modern browsers, and bundlers take care of it if we need to support older browsers (so in UMD bundle it's done twice currently). (https://stackoverflow.com/a/76851585)
Remove the use of Error.captureStackTrace, this is done by the base Error constructor, and it's only available in V8 engine based browsers/runtimes
Only catch the error from fetch to re-throw it as MeiliSearchRequestError, other potential errors should propagate as they're either truly unexpected or are thrown by us, simplifying error handling and not putting unexpected errors under the MeiliSearchError umbrella
Rename MeiliSearchErrorInfo type to MeiliSearchErrorResponse
Other minor changes/improvements

NOTE: Tests are horrifying, I didn't change all that much in src, but I had to change almost every test and by quite a bit. Testing is what I should aim to improve ASAP.

codecov · 2024-05-22T12:35:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.94%. Comparing base (01f51b4) to head (4b3dc78).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1656      +/-   ##
==========================================
- Coverage   97.44%   96.94%   -0.51%     
==========================================
  Files          22       21       -1     
  Lines         861      818      -43     
  Branches       93       78      -15     
==========================================
- Hits          839      793      -46     
- Misses         21       25       +4     
+ Partials        1        0       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

flevi29 · 2024-05-23T11:44:05Z

Breaking changes:

Rename MeiliSearchCommunicationError to MeiliSearchRequestError
Rename MeiliSearchErrorInfo type to MeiliSearchErrorResponse
MeiliSearchApiError now has a different property structure
- MeiliSearchErrorResponse is now in its cause property
- Instead of saving the HTTP status on the error, we save the whole Response on its response property for more available information (one of which is status, so error.response.status)
MeiliSearchRequestError now contains the whole caught error from fetch on its cause property instead of plucking some properties off of this error (this old way potentially losing information as we can not know the exact shape of this error)

flevi29 · 2024-05-25T12:39:47Z

I've looked into meilisearch-js-plugins, there is no handling of these special errors there, this should not break it in any way.

tests/errors.test.ts

flevi29 · 2024-05-29T20:23:53Z

tests/get_search.test.ts

+    expect(response).toHaveProperty('hits');
+    expect(Array.isArray(response.hits)).toBe(true);


Old way was checking with instanceof internally, but it is clearly sated in the docs that arrays are not to be checked this way: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/isArray#description
With builtin Node.js fetch this was failing with the old way.

flevi29 · 2024-05-29T20:28:53Z

These changes might appear daunting at first, but it's mostly tests, and because tests are repeated ad nauseam with very little generalization, you've seen 3 types of changes and you've seen them all.

src/http-requests.ts

tests/client.test.ts

flevi29 · 2024-06-01T06:18:11Z

Well, actually some more details about the cause property. Runtimes seem to print it and play nicely.

However, browsers don't give a damn. Only Firefox prints it at this moment, and unless cause is another Error object with a message property, it's not going to be of any service to you, absolutely beautiful work here:

But there's still hope, chromium is working on it: https://issues.chromium.org/issues/40182832
Maybe they will have the right ideas, unlike Firefox, even though they are massively late to the party, and let's not even talk about Safari, I'm sure they haven't yet started working on it.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause#browser_compatibility

Still, it's not like the old errors we have had any of their bespoke properties printed, so it's still an improvement, and it's future-proof.

brunoocasali

Hi @flevi29 thanks a lot for this PR!

I just want to ask you to create a small guide to the user if there is any upgrade path. Since you told us that there is no breaking in the js-plugins, I guess it should be shorter. But in any case I need something to put into the release :)

curquiza · 2024-07-24T12:24:01Z

bors merge

meili-bors · 2024-07-24T12:29:00Z

Build succeeded:

flevi29 · 2024-08-15T07:40:41Z

Hi @brunoocasali sorry for the late response.

Errors now make use of the standardized cause property.

MeiliSearchApiError
- is thrown when response.ok is false
- message -> cause.message
- code -> cause.code
- type -> cause.type
- link -> cause.link
- in case the request doesn't return a response body (for instance in case of an internal server error 500), cause remains undefined
- httpStatus -> response.status
- message, in case the response body is not empty, is its message property, otherwise HTTP status + status text
- response property contains the whole fetch Response
- MeiliSearchErrorInfo type renamed to MeiliSearchErrorResponse
MeiliSearchCommunicationError is removed

New:

MeiliSearchRequestError
- Is thrown when fetch throws for all of the reasons listed here
- cause property contains the error thrown by fetch

1695: Update version for the next release (v0.42.0) r=brunoocasali a=meili-bot _This PR is auto-generated._ The automated script updates the version of meilisearch-js to a new version: "v0.42.0" CHANGELOGS 👇 This version introduces features released on Meilisearch v1.10.0 🎉 Check out the changelog of [Meilisearch v1.10.0](https://github.com/meilisearch/meilisearch/releases/tag/v1.10.0) for more information on the changes. ## ⚠️ Breaking changes * Improve errors (#1656) `@/flevi29` More details [here](#1656 (comment)) * Changes related to Hybrid search (experimental) for the REST embedder (#1692) `@/mdubus` - Removed parameters: `query`, `inputField`, `inputType`, `pathToEmbeddings` and `embeddingObject`. - Replaced by `request` and `response` - New parameter: `headers` ## 🚀 Enhancements * Hybrid search improvements (#1692) `@/mdubus` - Add `url` parameter to the OpenAI embedder - `dimensions` is now available as an optional parameter for `ollama` embedders. * Add federated search parameters (#1689) `@/flevi29` ```js client.multiSearch({ federation: {}, queries: [ { indexUid: 'movies', q: 'batman', limit: 5, }, { indexUid: 'comics', q: 'batman', limit: 5, }, ] }) ``` * Add capabilities to update documents by function (#1691) `@/flevi29` ```js index.updateDocumentsByFunction({ context: { ctx: 'Harry' }, filter: 'id = 4', function: 'doc.comment = `Yer a wizard, ${context.ctx}!`', }) ) ``` * Add language settings (#1693) `@/flevi29` ```js client.index('INDEX_NAME').updateLocalizedAttributes([ { attributePatterns: ['jpn'], locales: ['*_ja'] }, ];) ``` * Add `locale` search parameter (#1693) `@/flevi29` ```js client.index('INDEX_NAME').search('進撃の巨人', { locales: ['jpn'] }) ``` ## ⚙️ Maintenance/misc * Add JS hosted documentation (#1678) `@/amit-ksh` Co-authored-by: meili-bot <[email protected]>

flevi29 linked an issue May 22, 2024 that may be closed by this pull request

Fetch error handling is incorrect #1655

Closed

flevi29 mentioned this pull request May 23, 2024

Remove cross-fetch dependency #1658

Closed

flevi29 requested a review from brunoocasali May 23, 2024 07:25

flevi29 added the breaking-change The related changes are breaking for the users label May 23, 2024

flevi29 added 7 commits May 23, 2024 14:53

Improved errors and error handling in http-requests

77fa8e1

Adapted tests in client.test.ts, added more metadata to errors

66a6c02

Adapted most tests to changes

778bd06

Adapted error tests, dropped one test case as it no longer applies

6ed75e0

Formatting and linting applied

aa5e8e1

Fixed jsdom trying to polyfill AbortController

12dad0e

Fixed array checks in tests

96d5ba7

flevi29 force-pushed the improve-errors branch from 2cb6f8a to 96d5ba7 Compare May 23, 2024 11:54

flevi29 mentioned this pull request May 26, 2024

Remove prettier custom options #1663

Merged

flevi29 added 2 commits May 29, 2024 23:00

Format to try and ease merge to main

bc4ebb4

Merge with format changes

4e55a32

flevi29 commented May 29, 2024

View reviewed changes

tests/errors.test.ts Show resolved Hide resolved

flevi29 commented May 29, 2024

View reviewed changes

Delete accidentally re-included file from merge

d3f490f

flevi29 commented May 30, 2024

View reviewed changes

src/http-requests.ts Show resolved Hide resolved

flevi29 commented May 30, 2024

View reviewed changes

tests/client.test.ts Show resolved Hide resolved

Merge branch 'main' into improve-errors

4b3dc78

brunoocasali approved these changes Jul 24, 2024

View reviewed changes

flevi29 closed this Jul 24, 2024

meili-bors bot merged commit 71dcebe into meilisearch:main Jul 24, 2024
7 of 9 checks passed

curquiza mentioned this pull request Aug 20, 2024

Update version for the next release (v0.42.0) #1695

Merged

sokl-octo mentioned this pull request Sep 13, 2024

fix(deps): update all non-major dependencies (minor) DNUM-SocialGouv/1j1s-etl#553

Open

1 task

KaliforniaShell mentioned this pull request Oct 2, 2024

[Snyk] Upgrade meilisearch from 0.35.1 to 0.42.0 KaliforniaShell/discord.js#3

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve errors #1656

Improve errors #1656

flevi29 commented May 22, 2024 •

edited

Loading

codecov bot commented May 22, 2024 •

edited

Loading

flevi29 commented May 23, 2024 •

edited

Loading

flevi29 commented May 25, 2024

flevi29 May 29, 2024 •

edited

Loading

flevi29 commented May 29, 2024

flevi29 commented Jun 1, 2024 •

edited

Loading

brunoocasali left a comment

curquiza commented Jul 24, 2024

meili-bors bot commented Jul 24, 2024

flevi29 commented Aug 15, 2024 •

edited

Loading

		expect(response).toHaveProperty('hits');
		expect(Array.isArray(response.hits)).toBe(true);

Improve errors #1656

Improve errors #1656

Conversation

flevi29 commented May 22, 2024 • edited Loading

Pull Request

Related issues

What does this PR do?

codecov bot commented May 22, 2024 • edited Loading

Codecov Report

flevi29 commented May 23, 2024 • edited Loading

flevi29 commented May 25, 2024

flevi29 May 29, 2024 • edited Loading

Choose a reason for hiding this comment

flevi29 commented May 29, 2024

flevi29 commented Jun 1, 2024 • edited Loading

brunoocasali left a comment

Choose a reason for hiding this comment

curquiza commented Jul 24, 2024

meili-bors bot commented Jul 24, 2024

flevi29 commented Aug 15, 2024 • edited Loading

flevi29 commented May 22, 2024 •

edited

Loading

codecov bot commented May 22, 2024 •

edited

Loading

flevi29 commented May 23, 2024 •

edited

Loading

flevi29 May 29, 2024 •

edited

Loading

flevi29 commented Jun 1, 2024 •

edited

Loading

flevi29 commented Aug 15, 2024 •

edited

Loading