deakjahn / flutter_dropzone

A drag-and-drop Flutter plugin (Web). Only web and only from outside into Flutter.
https://pub.dev/packages/flutter_dropzone
85 stars 36 forks source link

Migrate to package:web #75

Closed dxvid-pts closed 5 months ago

dxvid-pts commented 5 months ago

Flutter web apps are required to migrate from dart:html to package:web to use WASM. I can help writing a PR if help is wanted.

See https://docs.flutter.dev/platform-integration/web/wasm and https://pub.dev/packages/web for more information.

deakjahn commented 5 months ago

I'm not sure this is particularly important before the feature appears in the stable version. Do you happen to have an idea of when that might happen?

dxvid-pts commented 5 months ago

In the migration guide (https://dart.dev/interop/js-interop/package-web) they say plugins depending on dart:html should migrate asap. Also a lot of first party packages already migrated.

In the blog post today they said it will be released later this year (https://medium.com/dartlang/dart-3-3-325bf2bf6c13)

deakjahn commented 5 months ago

OK, let's see. I also have other packages here and there using html, so this could be a good testing ground... :-)

deakjahn commented 5 months ago

Well, try it, if you think so, but for my first attempt, this new package is very far from being a drop-in, or even functional replacement. It seems to lack many things previously working, despite the documentation claiming otherwise. I couldn't even set the style of a newly created web.Element (https://github.com/dart-lang/web/issues/176).

Not to mention that a huge list of classes that were provided by the previous approach are all gone now and you practically need to revert everything back to old, plain JavaScript code in Dart disguise). WASM is OK but I can't really understand the reason for this huge step backwards...

deakjahn commented 5 months ago

Some fragments:

  container = web.HTMLDivElement()
    ..id = id
    ..style.pointerEvents = 'auto'
    ..style.border = 'none'
    // idea from https://keithclark.co.uk/articles/working-with-elements-before-the-dom-is-ready/
    ..append(web.HTMLStyleElement()..innerText = '@keyframes $id-animation {from { clip: rect(1px, auto, auto, auto); } to { clip: rect(0px, auto, auto, auto); }}')
    ..style.animationName = '$id-animation'
    ..style.animationDuration = '0.001s'
    ..style.width = '100%'
    ..style.height = '100%'
    ..addEventListener('animationstart', _startCallback.toJS);
}

and

void _startCallback(web.Event event) {
  _nativeCreate(
    container,
    allowInterop(_onLoaded),
    allowInterop(_onError),
    allowInterop(_onHover),
    allowInterop(_onDrop),
    allowInterop(_onDropInvalid),
    allowInterop(_onDropMultiple),
    allowInterop(_onLeave),
  );
  if (mime != null) setMIME(mime!);
  if (operation != null) setOperation(operation!);
  if (cursor != null) setCursor(cursor!);
}

and

  picker.onchange = (_) {
    completer.complete(picker.files);
    if (isSafari) picker.remove();
  }.toJS;
  picker.oncancel = (_) {
    completer.complete([]);
    if (isSafari) picker.remove();
  }.toJS;
deakjahn commented 5 months ago

Also, the picker now returns a special list of web.File that can read the blob itself, without the current helper function (and probably without using FileReader() at all). All in all, although the few lines above look simple enough, the whole change might be much deeper than that.

dxvid-pts commented 5 months ago

Not to mention that a huge list of classes that were provided by the previous approach are all gone now and you practically need to revert everything back to old, plain JavaScript code in Dart disguise). WASM is OK but I can't really understand the reason for this huge step backwards...

So your current opinion is to wait? In my experience the biggest problem was the lack of documentation, not that features didn't exist so maybe it will improve over time? I don't have time pressure on this so I am fine with whatever direction you are going for the project.

Another option would be to particially migrate as you can use both systems in the same file.

Thanks for your work btw!

deakjahn commented 5 months ago

I only tried the superficial stuff, to see the new syntax. Most of it seems to work, syntactically at least, apart from the Url helper functions, I couldn't yet identify where they went. Also, the current @JS() annotations are no longer valid and I don't know what to use in lieu. They simply marked the actual JS name of a function so that we can call it _nativeSetCursor() in Dart and it gets compiled to a setCursor() in JS. This was mainly useful because we had a SetCursor() in the Dart code as well. If there's no current equivalent, a possibility is to rename everything on the JS side with a suffix like SetCursorJS() but simply, while all these steps are possible, I really can't see why the Flutter team pushes something as immature as this seems to be. :-)

I don't have any particular direction to follow with the project. It's specifically a web solution, not an universal drag-and-drop, that's stated in the docs. It clearly has limitations but these come from the browsers, they're not our fault (like not tolerating a large number of zones, eg. a long listview where every item is a separate drop zone). So, the direction is to simply keep it afloat.

I don't think the actual decision could come before working on it first. After the modifications, when it already compiles, it should be tested whether it still works or not. :-) Do you think you could afford some time to make the changes above, decorating all types with web., maybe leaving Url untouched now (eg. createObjectUrlFromBlob as a string doesn't appear in the new web package anywhere), renaming the JS functions as above so that the annotations are no longer required, to see whether it sinks immediately or not?

If we later see that the data returned (for instance, files) have changed in any way, then we have to make a breaking change, anyway, with major version change, keeping both versions. That would certainly be something I'd prefer to avoid.

deakjahn commented 5 months ago

Ok, I went a bit further. If you check this out, there are two major issues here:

lib.zip

getFileData() and getFileStream() are temporary, we need it to run to see how that works now. We might be able to solve it in a simpler way than before, there might be some built-in stream support now.

dxvid-pts commented 5 months ago

Ok nice work! I do have a solution for createObjectUrlFromBlob which I found by digging through PRs migrating packages maintained by the flutter team.

final blob = web.Blob(<JSUint8Array>[bytes.toJS].toJS);
final blobUrl = web.URL.createObjectURL(blob as dynamic);

I opened a work in progress PR with your changes. When I have more free time I will continue your work

deakjahn commented 5 months ago

(Moving to the PR thread.)