-
Notifications
You must be signed in to change notification settings - Fork 109
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 JS value to Dart conversion when receiving from a web socket #298
Merged
Merged
Changes from 6 commits
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 78387a6
Address comment
osa1 1916b79
Remove type annotation
osa1 10ec2ca
Update byte buffer to uint8list conversion
osa1 6bbe316
Merge remote-tracking branch 'origin/master' into osa1/js_value_fix
osa1 3bdf975
Add fix to allow tests to pass with Wasm!
kevmoo 28b48f2
oops
kevmoo 08fdebf
Fix Wasm spelling
osa1 a5011a2
Update conversion
osa1 31db374
Convert ArrayBuffer to Uint8List
osa1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
It looks like the fix is to
dartify
non-ArrayBuffer
s as well, which should only ever be a JS string iiuc. Is it worth adding an assert in anelse
condition to ensure that the result is aString
? This may be useful to avoid passing in unexpected Dart values down.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.
🤷
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.
With @osa1 we were looking at this today and the browser API says MessageEvent.data can be:
But it seemed the existing code somewhat assumed it's not going to get a
Blob
but only string or array bufferThere 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.
Expected JS type of binary messages is specified when constructing a web socket channel using this type:
web_socket_channel/lib/html.dart
Lines 177 to 184 in df096a9
Blob
(I don't know what would be the Dart type for this) orArrayBuffer
(which this library converts toUint8List
).When I searched for
BinaryType
(ag -w --dart BinaryType
) I can't find any uses in SDK or devtools, so it seems to me like we use the defaultBinaryType
which isArrayBuffer
. That's why the use site doesn't handle theBlob
case.(Relevant MDN: https://developer.mozilla.org/en-US/docs/Web/API/WebSocket/message_event#event_properties)
So we can't assert here that the data is either
JSArrayBuffer
or aString
, it can also beBlob
.We could assert that the data is one of
JSArrayBuffer
,JSString
, or the JS type forBlob
.I think it makes sense to avoid
dartify
here (which will do more type tests than necessary), and explicitly cover only these three cases. If thedata
is not a string orArrayBuffer
we can assert that it's aBlob
.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.
@srujzs how should we convert the
Blob
data to Dart? This can happen when you passBinaryType.blob
toHtmlWebSocketChannel.connect
.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.
If I'm interpreting the code in
vm_service
correctly here, it doesn't look like we handleBlob
s at all. On dart2js and ddc, we havedart:html
, which includesBlob
as a natively intercepted type. None of the type checks in the vm_service should apply to this type.Blob
is now inpackage:web
as a@staticInterop
type. It's a JS object which doesn't have an equivalent Dart type since it's not a typed array or any of the other JS types.Since we don't handle
Blob
s here to begin with, the correct migration might be to assert that it's one of those three types, and in the case ofBlob
s, pass it directly to the vm_service and have it log that it's an unknown message type.If we want, we can extract the contents of the
Blob
into a typed array and use that, but it doesn't look like the code was doing that before: https://developer.mozilla.org/en-US/docs/Web/API/Blob#extracting_data_from_a_blob.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.
Do you mean pass the
dartify
ofBlob
, or theJSValue
for it?I think it should be the former, but want to make sure.
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.
dartify
shouldn't do anything for aBlob
as it's not any of the primitive types or typed arrays, so either way should produce the sameJSValue
.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.
OK, in that case I think the current changes are fine?
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's up to you if you want to assert the types of
data
, but I think the functional changes are fine.