-
-
Notifications
You must be signed in to change notification settings - Fork 40
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
feat: add error codes #556
Conversation
@@ -97,6 +98,8 @@ def _request( | |||
) -> Union[T, None]: | |||
url = f"{self._url}/{path}" | |||
headers = {**self._headers, **(headers or {})} | |||
if headers.get(API_VERSION_HEADER_NAME) is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks incorrect. If there's no API_VERSION_HEADER_NAME
the server is probably on an old version (or potentially CORS misconfigured). In that case the default shouldn't be 2024-01-01 but rather the initial unspecified API version.
You can represent this as 1970-01-01
or something equivalent to it. None is fine too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the equivalent of what's in the JS library https://github.com/supabase/auth-js/blob/master/src/lib/fetch.ts#L137-L139, so is the JS library code incorrect?
data = error.response.json() | ||
|
||
error_code = None | ||
response_api_version = parse_response_api_version(error.response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably this parse_response_api_version
should return Unix timestamp 0 and your code below will magically work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean here? If I put 0 just as with the JS library it will fall through to the second part of the if
statement which is the elif
. Can you elaborate more so that it's clear what could go wrong here please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
707c176
to
cbbcd06
Compare
8b4dbb5
to
7f67ffd
Compare
What kind of change does this PR introduce?
Adds support for error codes. All
AuthError
descendants will now have acode
property which will encode (when present and supported by the server) the reason why the error occurred.What is the current behavior?
No error codes in AuthError
What is the new behavior?
Error codes in AuthError
Additional context
In relation to:
supabase/auth#1377
supabase/auth-js#855
supabase/auth#915