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

Make throwing errors on non-200 responses optional #61

Open
JuroUhlar opened this issue Feb 24, 2023 · 0 comments
Open

Make throwing errors on non-200 responses optional #61

JuroUhlar opened this issue Feb 24, 2023 · 0 comments

Comments

@JuroUhlar
Copy link
Contributor

JuroUhlar commented Feb 24, 2023

This is just a suggestion on how to make the API slightly more ergonomic for some users (partially subjective).

I was trying to validate a requestId, I did not check the SDK docs, I was just testing the API in Postman and my first instinct was to do it like this:

 const client = new FingerprintJsServerApiClient({ region: Region.Global, apiKey: SERVER_API_KEY });
    const eventResponse = await client.getEvent(requestId);

    // TS Error: Property 'error' does not exist on type '{ products?: { identification?: ....
    if ( eventResponse.error) {
    ...

The reason this does not work is that any non-200 response is caught as thrown as an error, so checking for non-existing requestId must be done on the error object inside a catch block. This works and is documented, but I think it might be unexpected to at least some portion of users. The natural expectation is that the SDK is just a wrapper over fetch and fetch only throws errors when the request itself fails (just extrapolating from my own experience here).

Sometimes, I totally expect the server to tell me it didn't find what I was looking for and it's not great DX to deal with that in a catch block:

  • It breaks the natural logic flow
  • The error object inside catch is not properly typed, so I need to figure out its shape using docs/experiments.

axios behaves the same way, but people found it confusing and they eventually fixed it by providing a validateStatus config option to turn the errors off.

Something potentially worth considering here as well (not necessarily making a breaking change but providing a way to change the default behavior).

The getEvent method could then return Promise<EventResponse | ErrorResponse> (same for getVisitorHistory) which would naturally nudge users to check for a non-200 response in a type-safe way (something that they need to do for every single use case).

@JuroUhlar JuroUhlar reopened this Feb 24, 2023
@JuroUhlar JuroUhlar changed the title getEvent return type should include possible error response getEvent could resolve a 404 response instead of throwing an error Feb 24, 2023
@JuroUhlar JuroUhlar changed the title getEvent could resolve a 404 response instead of throwing an error Make throwing errors on non-404 responses optional Feb 27, 2023
@JuroUhlar JuroUhlar changed the title Make throwing errors on non-404 responses optional Make throwing errors on non-200 responses optional Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant