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: compile with webdev #653

Merged
merged 3 commits into from
Oct 5, 2023
Merged

fix: compile with webdev #653

merged 3 commits into from
Oct 5, 2023

Conversation

Vinzent03
Copy link
Collaborator

@Vinzent03 Vinzent03 commented Oct 2, 2023

Bug fix

What is the current behavior?

  • We only catch SocketException, which in only thrown in native Dart, but not for example in dart2js
  • We use File from dart:io

What is the new behavior?

  • We instead use ClientException, which is also thrown in js runtime as documented here
  • Created a stub for File to be able to compile the package with webdev

Additional context

This was meant to fix #641, but this would require the removal of File, which im very unsure about. I think it makes things MUCH easier to be able to work with File on native platform, but somehow not being able to compile this to dart web is a bummer. I'm wondering why this package compiles for Flutter web though.
close #641

@dshukertjr
Copy link
Member

It sounds like dart2js support will not be possible without removing the use of File, correct?

How about we changing the .storage.upload() method to accept a Uint8List and get rid of the .storage.uploadBinary() method. It's a bit disruptive, but I always thought having two methods is a bit confusing. It might provide better DX if we had a single method for those storage operations.

@dshukertjr
Copy link
Member

It might be also pretty disruptive to drop support for uploading File object, so might not be a good idea. I personally wouldn't worry too much about supporting dart2js though. The use cases are very limited, and the SDK works just fine on Flutter Web. I'm more than happy to just close #641 as won't do.

@Vinzent03
Copy link
Collaborator Author

I think I was able to fix the issue 🎉 I created a stub for File which is used on js platform. The webdev build went well with only stubbing the used methods. This is actually no breaking change, so we could also merge this in main. 🤔

@@ -951,7 +951,7 @@ class GoTrueClient {
notifyAllSubscribers(AuthChangeEvent.tokenRefreshed);
_refreshTokenCompleter!.complete(authResponse);
return authResponse;
} on SocketException {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused about what is going on here. How can the thrown error type change without changing anything within the try block? Wouldn't .request() call on line 942 still throw SocketException when there is an issue with the network?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From the doc

If there is a socket-level failure when communicating with the server (for example, if the server could not be reached), IOClient will emit a ClientException that also implements SocketException. This allows callers to get more detailed exception information for socket-level failures, if desired.

So any SocketException from the HttpClient gets caught and put into a ClientException that also implements SocketException, so you can use both to catch the exception. See their send implementation.

@Vinzent03
Copy link
Collaborator Author

I moved the commits now to the main branch, so we can ship this as well to v1.

@Vinzent03 Vinzent03 changed the title fix: switch to ClientException fix: compile with webdev Oct 5, 2023
@Vinzent03 Vinzent03 merged commit 2324228 into main Oct 5, 2023
14 checks passed
@Vinzent03 Vinzent03 deleted the fix/io branch October 5, 2023 11:39
dshukertjr added a commit that referenced this pull request Oct 18, 2023
* feat: send messages via broadcast endpoint (#654)

* feat: send messages via broadcast endpoint

* chore: allow lower http version

* fix: fixes and add tests

* test: fix

* fix: compile with webdev (#653)

* chore(release): publish packages (#655)

- realtime_client@1.3.0
 - supabase@1.11.7
 - supabase_flutter@1.10.20

* chore(release): publish packages (#656)

- gotrue@1.12.3
 - storage_client@1.5.3
 - supabase@1.11.8
 - supabase_flutter@1.10.21

* fix(gotrue): remove import of dart:io from gotrue_client.dart (#659)

Remove import of dart:io from gotrue_client.dart

* chore(release): publish packages (#663)

- gotrue@1.12.4
 - supabase@1.11.9
 - supabase_flutter@1.10.22

* test: use ClientException

* test: remove unused import

---------

Co-authored-by: Tyler <18113850+dshukertjr@users.noreply.github.com>
Co-authored-by: Kabo <20254485+kaboc@users.noreply.github.com>
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.

Cannot build web app with pure Dart
2 participants