-
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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,10 @@ | |
import 'dart:async'; | ||
import 'dart:convert'; | ||
import 'dart:html'; | ||
import 'dart:js_util' as js_util; | ||
|
||
import 'package:js/js.dart'; | ||
import 'package:js/js_util.dart'; | ||
import 'package:logging/logging.dart'; | ||
import 'package:pool/pool.dart'; | ||
import 'package:stream_channel/stream_channel.dart'; | ||
|
@@ -64,13 +67,7 @@ class SseClient extends StreamChannelMixin<String?> { | |
// By default the SSE client uses keep-alive. | ||
// Allow for a retry to connect before giving up. | ||
_errorTimer = Timer(const Duration(seconds: 5), () { | ||
_incomingController.addError(error); | ||
close(); | ||
if (!_onConnected.isCompleted) { | ||
// This call must happen after the call to close() which checks | ||
// whether the completer was completed earlier. | ||
_onConnected.completeError(error); | ||
} | ||
_closeWithError(error); | ||
}); | ||
} | ||
}); | ||
|
@@ -103,6 +100,16 @@ class SseClient extends StreamChannelMixin<String?> { | |
_outgoingController.close(); | ||
} | ||
|
||
void _closeWithError(Object error) { | ||
_incomingController.addError(error); | ||
close(); | ||
if (!_onConnected.isCompleted) { | ||
// This call must happen after the call to close() which checks | ||
// whether the completer was completed earlier. | ||
_onConnected.completeError(error); | ||
} | ||
} | ||
|
||
void _onIncomingControlMessage(Event message) { | ||
var data = (message as MessageEvent).data; | ||
if (data == 'close') { | ||
|
@@ -133,12 +140,48 @@ class SseClient extends StreamChannelMixin<String?> { | |
_logger.warning('Invalid argument: $e'); | ||
} | ||
try { | ||
await HttpRequest.request('$_serverUrl&messageId=${++_lastMessageId}', | ||
method: 'POST', sendData: encodedMessage, withCredentials: true); | ||
} catch (e) { | ||
_logger.severe('Failed to send $message:\n $e'); | ||
close(); | ||
final url = '$_serverUrl&messageId=${++_lastMessageId}'; | ||
await _fetch( | ||
url, | ||
FetchOptions( | ||
method: 'POST', | ||
body: encodedMessage, | ||
credentialsOptions: | ||
CredentialsOptions(credentials: 'include'))); | ||
} catch (error) { | ||
final augmentedError = 'SSE client failed to send $message:\n $error'; | ||
_logger.severe(augmentedError); | ||
_closeWithError(augmentedError); | ||
} | ||
}); | ||
} | ||
} | ||
|
||
// 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Yes it does! Switched to that |
||
resourceUrl, | ||
options, | ||
])); | ||
} | ||
|
||
@JS() | ||
@anonymous | ||
class FetchOptions { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We might need to move these to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've made them private |
||
external String get method; // e.g., 'GET', 'POST' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Good point, removed the getters |
||
external CredentialsOptions get credentialsOptions; | ||
external String? get body; | ||
external factory FetchOptions({ | ||
String method, | ||
CredentialsOptions credentialsOptions, | ||
String? body, | ||
}); | ||
} | ||
|
||
@JS() | ||
@anonymous | ||
class CredentialsOptions { | ||
external String get credentials; // e.g., 'omit', 'same-origin', 'include' | ||
external factory CredentialsOptions({String credentials}); | ||
} |
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!