-
Notifications
You must be signed in to change notification settings - Fork 721
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: TypeError: Instance of 'NativeByteBuffer': type 'NativeByteBuffer' is not a subtype of type 'List'. #1496
Conversation
When built in the html web-renderer, an error may occur: TypeError: Instance of 'NativeByteBuffer': type 'NativeByteBuffer' is not a subtype of type 'List<int>'. Therefore, we need to make a type discrimination of the reader.result.
Apply suggestions from code review Co-authored-by: Navaron Bracke <[email protected]> readerResult
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.
Some code is now redundant with the new approach.
lib/_internal/file_picker_web.dart
Outdated
if (readerResult == null) { | ||
continue; | ||
} | ||
if (readerResult is ByteBuffer) { |
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.
This if-block can be removed, as you now handle it below, with if (readerResult.instanceOfString('JSArrayBuffer')) {
. This is because the JSArrayBuffer
maps to a ByteBuffer in Dart.
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.
Apologies, I wrote a typo there. JS has a ArrayBuffer
type, which Dart's JS interop identifies as a JSArrayBuffer
for the JS <-> Dart types distinction.
If you do readerResult.instanceOfString('ArrayBuffer')
is that true
?
We can handle the ByteBuffer
case directly as well, like you did before. If we use the readerResult
as local variable, the type system can help in promoting the type, so you don't need a cast.
I guess we can get a ByteBuffer directly in some cases (dart2js or DDC might special case types from dart:typed_data
)
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, you are right, readerResult.instanceOfString('ArrayBuffer')
is true.
If without a cast, although the type system can help in promoting the type, the IDE can not. That's to say, readerResult
does not have a function named asUint8List
, but (readerResult as ByteBuffer)
does have.
So I think using a cast is clearer.
I don't know whether using instanceOfString
can cover all aspects of the readerResult type.
I guess handling the ByteBuffer case directly is better.
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.
This is still not quite there yet.
The following should do it, though.
A short explanation:
Since we are running on the web, we are always dealing with the types in
https://github.com/dart-lang/sdk/blob/cf564396d263434c29efe984f1f2220814e5969a/sdk/lib/_internal/js_shared/lib/js_types.dart
as that is a guarantee by the DDC / dart2js compiler.
Since the FileReader.result
is JSAny?
we handle null first.
Now we still have a JSAny
which can be ArrayBuffer
or Array
in JS.
Since the isA<T extends JSAny>()
helper is not yet available, we use the JavaScript instanceof
, by calling out to instanceOfString()
instanceOfString('ArrayBuffer')
-> (which maps toNativeByteBuffer
in the types above, which is why you see theNativeByteBuffer
)instanceOfString('Array')
-> here we assume we got a JS Array with integers, instead of an ArrayBuffer
In the future, the isA<T extends JSAny>()
helper will help with type promotion on the variable, but since we don't have that yet, we have to use instanceof
to be safe.
if (readerResult is ByteBuffer) { | |
Stream<List<int>> _openFileReadStream(File file) async* { | |
final reader = FileReader(); | |
int start = 0; | |
while (start < file.size) { | |
final end = start + _readStreamChunkSize > file.size | |
? file.size | |
: start + _readStreamChunkSize; | |
final blob = file.slice(start, end); | |
reader.readAsArrayBuffer(blob); | |
await EventStreamProviders.loadEvent.forTarget(reader).first; | |
final JSAny? readerResult = reader.result; | |
if (readerResult == null) { | |
continue; | |
} | |
// TODO: use `isA<JSArrayBuffer>()` when switching to Dart 3.4 | |
// Handle the ArrayBuffer type. This maps to a `ByteBuffer` in Dart. | |
if (readerResult.instanceOfString('ArrayBuffer')) { | |
yield (readerResult as JSArrayBuffer).toDart.asUint8List(); | |
start += _readStreamChunkSize; | |
continue; | |
} | |
// TODO: use `isA<JSArray>()` when switching to Dart 3.4 | |
// Handle the Array type. | |
if (readerResult.instanceOfString('Array')) { | |
// Assume this is a List<int>. | |
yield (readerResult as JSArray).toDart.cast<int>(); | |
start += _readStreamChunkSize; | |
} | |
} | |
} |
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.
Your explanation is very clear. I have applied your suggestions. Thank you! :)
lib/_internal/file_picker_web.dart
Outdated
// Assume this is a List<int>. | ||
yield (readerResult as JSArray).toDart.cast<int>(); | ||
start += _readStreamChunkSize; | ||
continue; |
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.
This continue;
is redundant.
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.
done
lib/_internal/file_picker_web.dart
Outdated
start += _readStreamChunkSize; | ||
continue; | ||
} | ||
// Default this is a List<int> |
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.
You already handled the List<int>
case with if (readerResult.instanceOfString('Array')) {
on line 230, so this block can be removed.
Co-authored-by: Navaron Bracke <[email protected]>
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.
LGTM
If you can update the patch version and add a changelog entry, then we can publish this fix.
When built in the html web-renderer, an error may occur:
TypeError: Instance of 'NativeByteBuffer': type 'NativeByteBuffer' is not a subtype of type 'List'.
Therefore, we need to make a type discrimination of the reader.result.