pub-dev icon indicating copy to clipboard operation
pub-dev copied to clipboard

Add badges for Wasm compatibility

Open johnpryan opened this issue 2 years ago • 29 comments

Flutter web plugins are currently using dart:html and package:js to wrap JavaScript APIs. The new version of Dart-JavaScript interop will depend on package:web and dart:js_interop, respectively.

This is a proposal to display a "Wasm compatible" badge if the package imports the correct libraries, and "Wasm incompatible" if it is using the old libraries.

cc: @kevmoo @joshualitt

johnpryan avatar Jun 21 '23 19:06 johnpryan

We're not quite ready to define the bits here, but it's good to have a placeholder!

kevmoo avatar Jun 21 '23 19:06 kevmoo

This should be possible by checking if the package depends on dart:html, dart:js, dart:js_util, or package:js?

Or any other dependency that does not support WASM (e.g. because it or one of its transitive dependencies depend on the above), right?

IchordeDionysos avatar Feb 15 '24 21:02 IchordeDionysos

@IchordeDionysos – yep!

kevmoo avatar Feb 15 '24 21:02 kevmoo

Hmm, okay, the tricky part is that pub.dev does not know whether the package depends on dart:xxx packages ... https://github.com/flutter/packages/blob/main/packages/video_player/video_player_web/lib/src/video_player.dart#L6

This might have been easily doable (if this was already indexed / available) in the pub.dev database.

IchordeDionysos avatar Feb 15 '24 22:02 IchordeDionysos

Hmm, okay, the tricky part is that pub.dev does not know whether the package depends on dart:xxx packages ...

Not sure if this is a typo or not, so just to be specific about it: pub.dev does package analysis (using package:pana) and part of it is to scan the dependency graph for dart:xxx library uses. We have started analysis for wasm-compatibility with the recent release, however it is not exposed on the UI yet.

isoos avatar Feb 15 '24 22:02 isoos

@isoos Actually, in my assumption, I thought the scan would not detect uses of dart:xxx, so wasn't a typo, just my lack of knowledge.

I came to that conclusion as usages of dart:xxx packages seem to be not exposed in the UI one pub.dev as well and I thought the reason is the data is not available.

e.g. video_player_web uses dart:html: https://github.com/flutter/packages/blob/main/packages/video_player/video_player_web/lib/video_player_web.dart#L6 But it's not listed as a dependency here: https://pub.dev/packages/video_player_web

Definitely nice to hear support for this badge is progressing 🙌

IchordeDionysos avatar Feb 15 '24 23:02 IchordeDionysos

however it is not exposed on the UI yet.

There is no filter yet, but this seems to work already 😍 https://pub.dev/packages?q=is%3Awasm-ready At least I can't find the video_player nor video_player_web package :)

If I'm getting too excited too early and the data is not yet fully up-to-date or if there's another reason it has yet to be added, I'm sorry :D

IchordeDionysos avatar Feb 15 '24 23:02 IchordeDionysos

But it's not listed as a dependency here: https://pub.dev/packages/video_player_web

We list only package dependencies on the package page (direct ones in the infobox, direct+transient in the package score tab). It may be useful to also expose the SDK library dependencies (dart:*) too, e.g. using dart:io may or may not be a concern for users of the package, but we don't have any immediate plan to implement these.

There is no filter yet, but this seems to work already

We are still evaluating if the analysis is correct and makes sense.

isoos avatar Feb 15 '24 23:02 isoos

We are still evaluating if the analysis is correct and makes sense.

Also, almost forgot: if you see something wrong with the tagging (either false positive or false negative), please report it (here or in package:pana).

isoos avatar Feb 15 '24 23:02 isoos

Yeah seen a few where it seemed incorrect, will compile a list.

IchordeDionysos avatar Feb 16 '24 07:02 IchordeDionysos

@IchordeDionysos This filter shows image_picker and other packages that still don’t support wasm yet or in-progress https://pub.dev/packages?q=is%3Awasm-ready

image_picker still in-progress https://github.com/flutter/flutter/issues/117022

amrgetment avatar Feb 16 '24 07:02 amrgetment

@isoos I think package:pana may need to check for dart:html
dart:js
dart:js_util
package:js in the package or its dependencies like @IchordeDionysos said

amrgetment avatar Feb 16 '24 07:02 amrgetment

Definition:

  • false positive: Falsely marked as WASM-ready
  • false negative: Falsely marked as non-WASM-ready

intl: false positive The package still seems to use dart:html

i18n_extension: false positive Also, packages depending on intl are listed as supported.

flutter_keyboard_visibility: false positive Depends on dart:html

intercom_flutter: false positive Depends on dart:js and dart:html

oauth2_client: false positive: Depends on a flutter_secure_storage which depends on flutter_secure_storage_web which depends on package:js

sensors_plus: false positive: Depends on both dart:html and dart:js_util

uni_links: false positive: Depends on dart:html

fetch_client: false negative ??? There is a pre-release version with WASM support, but still listed as unsupported? I guess that might be expected due to the pre-release

IchordeDionysos avatar Feb 16 '24 08:02 IchordeDionysos

Just uploaded https://pub.dev/packages/flutter_markdown – which I think is wasm-ready, but it's not being labeled

kevmoo avatar Feb 21 '24 20:02 kevmoo

Looking at uni_links I think we might not be handling federated plugins correctly.

I'm pretty sure that flutter.plugin.platforms.web.default_package causes some stuff to be generated, and our import analysis does not take that into account.

But if we were to use flutter.plugin.platforms in platform detection we might need reference documentation for flutter.plugin.platforms, otherwise it'll be very hard to maintain (or even know if it's correct). This, is perhaps something we should push for.

jonasfj avatar Jul 24 '24 08:07 jonasfj

I guess technically a federated plugin like uni_links can be used with wasm, it's just that you'd have to write your own platform implementation :see_no_evil: (not suggesting that the classification is ideal).

The plugin does not directly import dart:html so probably possible to override the default web implementation uni_links_web. It's probably even documented in the federated plugin docs somewhere.

jonasfj avatar Jul 24 '24 08:07 jonasfj

The thing is when you research a package you want it to work on all platforms and not have to write your own implementation for it.

IchordeDionysos avatar Jul 24 '24 09:07 IchordeDionysos

Yeah, but if I have to choose between false-positive / and true-negative, then in the case of platform tagging, false-positive isn't the worst.

But yes, we need to do better at understand flutter plugins.

jonasfj avatar Jul 25 '24 12:07 jonasfj

intl: false positive The package still seems to use dart:html

I'm actually not sure this is a false-positive.

We allow importing dart:html as long as it's not in the primary library. In this case that's package:intl/intl.dart.

It's okay for other (optional) libraries to import it, so package:intl/intl_browser.dart is okay.


For all platform detection we use the primary library (if there is no primary library, we require all libraries).

For platform detection we do this because we assume that the primary library will use configurable-imports. And we assume that other libraries might be used to expose additional platform specific functionality.

We could ofcourse, require that if you implement platform specific functionality in additional libraries, then you must use configurable imports, such that the same API exists on other platforms, but that instead those platform throws UnimplementError. (that'd be a change of opinion).

The other cases appears to be flutter plugins, I agree that we should be better at understanding flutter plugins. But I don't know if we want to block on this.

jonasfj avatar Jul 25 '24 12:07 jonasfj

Could we start with an FYI on the score tab to show how we're evaluating a package?

Might be a nice tip-toe into this space without affecting scores

kevmoo avatar Aug 13 '24 22:08 kevmoo

Not sure where the best place to report general false negatives are, maybe pana (eg. https://github.com/dart-lang/pana/issues/1324) would be more suited, but I think until packages can define that they do or do not support Wasm (as is the case with platforms) in their pubspec, then it will be difficult to do this accurately.

For example, 'package:logger' is reported as non-Wasm ready, because one of it's files/internal libraries does a conditional import of 'dart:io' (but the package is not a plugin, and this is the primary library), which means 'package:flutter_map' becomes non-Wasm ready, even though it is proved to build with Wasm.

Until maintainers get a way to report Wasm compatibility themselves (as with platforms), then awarding badges might be an issue. Or, pana needs to get better at platform detection for conditional imports. Or, maybe 'package:logger' is just written incorrectly for pana detection? Not sure.

JaffaKetchup avatar Sep 28 '24 20:09 JaffaKetchup

This should likely be copied to pkg:pana.

Thoughts @jonasfj ?

kevmoo avatar Sep 30 '24 04:09 kevmoo

Please file again pana. But really package:logger doesn't conditionally import dart:io. https://github.com/SourceHorizon/logger/blob/main/lib/src/outputs/advanced_file_output_stub.dart#L2

The problem is that importing dart:io on the web is allowed and works, so long as you don't use anything from it. Basically, you can use the types. But we can't detect whether you do that or not in pana, so we end up with this weird state where we report something as not supporting web because it imports dart:io and doesn't do anything with it (which technically works, and is useful in a very few scenarios where we you access to types from dart:io, say you do a custom implementation of File).

jonasfj avatar Sep 30 '24 10:09 jonasfj

Ah, I see, so the library is written somewhat incorrectly. What does pana do with conditional imports anyway The main point still stands I guess. I would like to mark flutter_map as Wasm compatible anyway.

JaffaKetchup avatar Sep 30 '24 11:09 JaffaKetchup

Unlike platforms where you can explicitly overwrite automatic detection, no such thing is supported for wasm. https://dart.dev/tools/pub/pubspec#platforms

@kevmoo, @sigurdm this is probably a bit of a hole. wasm isn't really a platform. I suppose we could allow overridding the detection with something like:

platform:
  web:
    wasm: true

I don't know, if we're ready to formalize what values goes under platform.<platform> yet.


@JaffaKetchup at the moment by strong suggestion would be to contributor to package:logger. They could probably remove the unnecessary import of dart:io. Cleaning it up will also help platform detection for other packages using logger.

jonasfj avatar Sep 30 '24 11:09 jonasfj

Yeah, something like that would be what I was looking for 👍

JaffaKetchup avatar Sep 30 '24 11:09 JaffaKetchup

Similar issues happen here:

False positive

maplibre_gl is marked wasm compatible on pub.dev. It doesn't include dart:io but uses maplibre_gl_web for its web implementation like a lot of other flutter plugins do. The problem is that maplibre_gl_web does not use compatible js interop. Therefore the [maplibre_gl] used directly by the user can't run on web with wasm either.

False negative

maplibre is marked wasm incompatible on pub.dev but runs fine on web with wasm. The web implementation gets registered like documented in the flutter plugin documentation and how it is done in a newly created flutter plugin project. However, the platform channel implementation is directly imported in the platform interface class. Pana detects this and follows the imports to the dart:io imports but does not detect the registered web implementation.

Package not compatible with runtime wasm
Because:
package:maplibre/maplibre.dart that imports:
package:maplibre/src/map.dart that imports:
package:maplibre/src/platform_interface.dart that imports:
package:maplibre/src/native/platform_native.dart that imports:
package:maplibre/src/native/widget_state.dart that imports:
dart:io

The pana can improve a lot in detecting false positives and false negatives but I'm not sure if it will be possible to be 100% accurate.

josxha avatar Sep 30 '24 11:09 josxha

maplibre_gl is marked wasm compatible on pub.dev. It doesn't include dart:io but uses maplibre_gl_web for its web implementation like a lot of other flutter plugins do.

maplibre_gl is a federated plugin, and only imports directly from https://pub.dev/packages/maplibre_gl_platform_interface, which is a generic interface for. One could (at least in theory) plug another wasm-compatible web-implementation in.

We could consider it not wasm-compatible if its default implementation is not wasm-compatible. But that is not the current behavior, and not clear that it is correct. (It would also severely make the platform detection less generic.)

sigurdm avatar Sep 30 '24 12:09 sigurdm

maplibre is marked wasm incompatible on pub.dev but runs fine on web with wasm.

The import here https://github.com/josxha/flutter-maplibre/blob/main/lib/src/platform_interface.dart#L3 should be made conditional, to make the static distinction clear to pana.

sigurdm avatar Sep 30 '24 12:09 sigurdm