Improve errors #1656
Improve errors #1656
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1656 +/- ##
==========================================
- Coverage 97.43% 96.93% -0.51%
==========================================
Files 22 21 -1
Lines 858 815 -43
Branches 86 78 -8
==========================================
- Hits 836 790 -46
- Misses 21 24 +3
Partials 1 1 ☔ View full report in Codecov by Sentry. |
|
Breaking changes:
|
|
I've looked into |
| test(`Not throw MeiliSearchCommunicationError when throwned error is MeiliSearchApiError`, async () => { | ||
| fetchMock.mockReject( | ||
| new MeiliSearchApiError( | ||
| { | ||
| message: 'Some error', | ||
| code: 'some_error', | ||
| type: 'random_error', | ||
| link: 'a link', | ||
| }, | ||
| 404, | ||
| ), | ||
| ); | ||
|
|
||
| const client = new MeiliSearch({ host: 'http://localhost:9345' }); | ||
| try { | ||
| await client.health(); | ||
| } catch (e: any) { | ||
| expect(e.name).toEqual('MeiliSearchApiError'); | ||
| } | ||
| }); | ||
|
|
||
| test('MeiliSearchApiError can be compared with the instanceof operator', async () => { | ||
| fetchMock.mockReject( | ||
| new MeiliSearchApiError( | ||
| { | ||
| message: 'Some error', | ||
| code: 'some_error', | ||
| type: 'random_error', | ||
| link: 'a link', | ||
| }, | ||
| 404, | ||
| ), | ||
| ); | ||
|
|
||
| const client = new MeiliSearch({ host: 'http://localhost:9345' }); | ||
| try { | ||
| await client.health(); | ||
| } catch (e: any) { | ||
| expect(e instanceof MeiliSearchApiError).toEqual(true); | ||
| } |
There was a problem hiding this comment.
This test case was removed, because it's no longer applicable, MeiliSearchApiError doesn't get caught anymore to be re-thrown unnecessarily.
| expect(response).toHaveProperty('hits'); | ||
| expect(Array.isArray(response.hits)).toBe(true); |
There was a problem hiding this comment.
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.
|
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. |
| const responseBody = await response.text(); | ||
| const parsedResponse = | ||
| responseBody === '' ? undefined : JSON.parse(responseBody); |
There was a problem hiding this comment.
Previously if response.json() failed, we swallowed the error. There are cases when the response body is empty, and is parsed to an empty string initially. That is an invalid JSON. This case was handled by simply swallowing the error, now it's handled explicitly, and if for whatever reason Meilisearch returns something non-JSON-valid other than an empty body, it will err, which is what we want, as that's entirely unexpected and should propagate.
| @@ -56,11 +56,11 @@ describe.each([ | |||
| apiKey: key, | |||
| requestConfig: { | |||
| headers: { | |||
| Expect: '200-OK', | |||
| 'Hello-There!': 'General Kenobi', | |||
|
Well, actually some more details about the
However, browsers don't give a damn. Only Firefox prints it at this moment, and unless
But there's still hope, chromium is working on it: https://issues.chromium.org/issues/40182832 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. |
Pull Request
Related issues
Fixes #1612, #1655
What does this PR do?
This PR aims to improve errors, so that they can contain all the necessary information, and more.
jsdomfrom replacingfetchandAbortControllerfor consistency with node tests, replacing previous solution where we removed the builtinfetchfrom node tests"abort-controller"package, it was only used in tests and nowAbortControlleris always availableMeiliSearchCommunicationErrortoMeiliSearchRequestError, as this error might not be entirely related to communication, but rather the request itselfcause, preserving the original error, simplifying things, taking advantage of modern browsers and runtimes actually printing this propertyObject.setPrototypeOfin 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)Error.captureStackTrace, this is done by the baseErrorconstructor, and it's only available in V8 engine based browsers/runtimesfetchto re-throw it asMeiliSearchRequestError, 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 theMeiliSearchErrorumbrellaMeiliSearchErrorInfotype toMeiliSearchErrorResponseNOTE: 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.