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.

Sign up for GitHub

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 JS value to Dart conversion when receiving from a web socket #298

Merged
merged 10 commits into from
Dec 9, 2023
Merged

Fix JS value to Dart conversion when receiving from a web socket #298

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1f34c30
Fix JS value to Dart conversion when receiving from a web socket
osa1 Nov 30, 2023
78387a6
Address comment
osa1 Nov 30, 2023
1916b79
Remove type annotation
osa1 Nov 30, 2023
10ec2ca
Update byte buffer to uint8list conversion
osa1 Nov 30, 2023
6bbe316
Merge remote-tracking branch 'origin/master' into osa1/js_value_fix
osa1 Nov 30, 2023
3bdf975
Add fix to allow tests to pass with Wasm!
kevmoo Dec 1, 2023
28b48f2
oops
kevmoo Dec 1, 2023
08fdebf
Fix Wasm spelling
osa1 Dec 1, 2023
a5011a2
Update conversion
osa1 Dec 8, 2023
31db374
Convert ArrayBuffer to Uint8List
osa1 Dec 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/html.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,13 +127,13 @@ class HtmlWebSocketChannel extends StreamChannelMixin
}

void _innerListen(MessageEvent event) {
final eventData = event.data;
final JSAny? eventData = event.data;
Copy link
Member

@mkustermann mkustermann Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do we really need the if / else here? Can this be just

  void _innerListen(MessageEvent event) {
    final JSAny? eventData = event.data;
    _controller.local.sink.add(eventData.dartify());
  }

?

Copy link
Member Author

Choose a reason for hiding this comment

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

That changes how ArrayBuffer is handled as dartify will give you a ByteBuffer instead of a Uint8List.

Maybe package maintainers can comment on whether that's OK.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the type test.

I don't know why we want to convert ArrayBuffer (ByteBuffer in Dart) to Uint8List, it's not documented anywhere in this library as far as I can see. The receiver end of this stream in vm_service is here: https://github.com/dart-lang/sdk/blob/26107a319a7503deafee404e3462644a873e2920/pkg/vm_service/lib/src/vm_service.dart#L1795-L1807

It's expecting (1) String (2) List<int> (3) ByteData, but as far as I understand from https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/message_event and this library it will never receive a ByteData (JS DataView). Maybe it needs to handle ByteBuffer instead of ByteData?

Object? data;
if (eventData.typeofEquals('object') &&
(eventData as JSObject).instanceOfString('ArrayBuffer')) {
data = (eventData as JSArrayBuffer).toDart.asUint8List();
} else {
data = event.data;
data = event.data.dartify();
osa1 marked this conversation as resolved.
Show resolved Hide resolved
}
_controller.local.sink.add(data);
}
Expand Down
Loading