-
Notifications
You must be signed in to change notification settings - Fork 24
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
Use Fetch API in SSE Client #66
Conversation
lib/client/sse_client.dart
Outdated
@@ -5,7 +5,10 @@ | |||
import 'dart:async'; | |||
import 'dart:convert'; | |||
import 'dart:html'; | |||
import 'dart:js_util' as js_util; |
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.
Why do we need this import in addition to the import on line 11?
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.
Ah no longer need either after addressing your other suggestions!
lib/client/sse_client.dart
Outdated
@JS() | ||
@anonymous | ||
class FetchOptions { | ||
external String get method; // e.g., 'GET', 'POST' |
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.
[nit] I don't think these getters are used in Dart. It shouldn't be necessary to define them if they aren't used - the constructor is the important part of this class definition
class FetchOptions {
external factory FetchOptions({
String method, // e.g., 'GET', 'POST'
CredentialsOptions credentialsOptions,
String? body,
});
}
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.
Good point, removed the getters
lib/client/sse_client.dart
Outdated
|
||
@JS() | ||
@anonymous | ||
class FetchOptions { |
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.
We might need to move these to lib/src
, or make them private. I don't think we want to expose JS interop types from this library.
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.
I've made them private
lib/client/sse_client.dart
Outdated
// Custom implementation of Fetch API until Dart supports GET vs. POST, | ||
// credentials, etc. See https://github.com/dart-lang/http/issues/595. | ||
Future<dynamic> _fetch(String resourceUrl, FetchOptions options) { | ||
return js_util.promiseToFuture(js_util.callMethod(globalThis, 'fetch', [ |
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.
Does it work to do
@JS('fetch')
external Object _fetchPromise(String resourceUrl, FetchOptions options);
Future<dynamic> _fetch(String resourceUrl, FetchOptions options) =>
promiseToFuture(_fetchPromise(resourceUrl, options));
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.
Yes it does! Switched to that
Use
fetch
requests instead of XHR requests insse_client
.Manifest V3 Chrome Extensions no longer supports XHR (see dart-lang/http#595 (comment)). Therefore, this PR switches
sse_client
over to usingfetch
requests to unblock the Dart Debug Extension.I'm also now closing the connection with an error when something goes wrong with the request. This makes it easier for clients to figure out why their connection was closed, eg: