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.

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

Merged
merged 9 commits into from
Apr 27, 2024

Conversation

vito-go
Copy link
Contributor

@vito-go vito-go commented Apr 20, 2024

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.

 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.
@vito-go vito-go changed the title fix: make a type discrimination of the reader.result. Fix: TypeError: Instance of 'NativeByteBuffer': type 'NativeByteBuffer' is not a subtype of type 'List'. Apr 24, 2024
lib/_internal/file_picker_web.dart Outdated Show resolved Hide resolved
vito-go and others added 2 commits April 25, 2024 19:40
Apply suggestions from code review

Co-authored-by: Navaron Bracke <brackenavaron@gmail.com>

readerResult
Copy link
Collaborator

@navaronbracke navaronbracke left a 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.

if (readerResult == null) {
continue;
}
if (readerResult is ByteBuffer) {
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, when I debug on my machine, it shows that readerResult.runtimeType is "ByteBuffer" and
readerResult.instanceOfString('JSArrayBuffer')-> false, readerResult.instanceOfString('Array')-> false.

readerResult

Copy link
Collaborator

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)

Copy link
Contributor Author

@vito-go vito-go Apr 25, 2024

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.

Copy link
Collaborator

@navaronbracke navaronbracke Apr 26, 2024

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()

  1. instanceOfString('ArrayBuffer')
    -> (which maps to NativeByteBuffer in the types above, which is why you see the NativeByteBuffer)
  2. 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.

Suggested change
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;
}
}
}

Copy link
Contributor Author

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! :)

// Assume this is a List<int>.
yield (readerResult as JSArray).toDart.cast<int>();
start += _readStreamChunkSize;
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This continue; is redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

start += _readStreamChunkSize;
continue;
}
// Default this is a List<int>
Copy link
Collaborator

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 <brackenavaron@gmail.com>
Copy link
Collaborator

@navaronbracke navaronbracke left a 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.

@navaronbracke navaronbracke merged commit 7b3eebf into miguelpruivo:master Apr 27, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants