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

fix: Added missing error codes for AuthException #995

Merged
merged 11 commits into from
Aug 12, 2024

Conversation

lawinski
Copy link
Contributor

@lawinski lawinski commented Aug 2, 2024

What kind of change does this PR introduce?

Add support for auth error codes

What is the current behavior?

There are no error code in auth exception

What is the new behavior?

Error codes added to AuthException

@lawinski lawinski mentioned this pull request Aug 2, 2024
@dshukertjr
Copy link
Member

Thanks for getting started on this one! Here is the JS PR for the same feature. If you could match the implementation as much as possible, that would be great!
supabase/auth-js#855

- Introduce api versioning
- Change AuthErrorCode to ErrorCode
@lawinski
Copy link
Contributor Author

lawinski commented Aug 3, 2024

Applied adjustments to the logic from JS implementation:

  • Modified the version management by wrapping the logic into a dedicated class and storing API versions in another. This allows for more type-safe access and consolidates all logic in one place. The core logic remains unchanged.
  • Added tests for GotrueFetch to cover different responses (MockedHttpClient added for this purpose).
  • Changed AuthErrorCode to ErrorCode.

Doubts:

  • Do not introduce exceptions that do not already exist (e.g., AuthInvalidTokenResponseError, AuthImplicitGrantRedirectError, AuthInvalidCredentialsError).
  • Setting up a default version header if it does not exist in the request method prevents the user from using the initial API version until a invalid one is provided. I’m unsure if this is a good assumption, but I guess you have already considered this. :)

PS. If the logic should match the JS implementation exactly, please let me know. If we are heading in that direction, I would appreciate guidance on the process of implementing new functionalities. Does this mean we always have to wait for the implementation in JS first?

Copy link
Member

@dshukertjr dshukertjr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lawinski Thanks for the amazing work! This PR is awesome! I edited the code in a way that I think would be more aligned with the JS SDK and might be easier to maintain, but would love to hear what you think!

If the logic should match the JS implementation exactly, please let me know. If we are heading in that direction, I would appreciate guidance on the process of implementing new functionalities.

Generally yes. The closer the implementation is to the JS SDK, the easier to maintain, and also could avoid having to deviate the SDK from the JS one because of future changes that were not anticipated.

Does this mean we always have to wait for the implementation in JS first?

Also generally yes.

@lawinski
Copy link
Contributor Author

lawinski commented Aug 6, 2024

@dshukertjr I'm okay with the changes :) LGTM

Copy link
Collaborator

@bdlukaa bdlukaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

packages/gotrue/lib/src/fetch.dart Show resolved Hide resolved
@dshukertjr dshukertjr merged commit 4e0270a into supabase:main Aug 12, 2024
9 checks passed
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

Successfully merging this pull request may close these issues.

3 participants