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

Add badges for Wasm compatibility #6785

Open
johnpryan opened this issue Jun 21, 2023 · 29 comments
Open

Add badges for Wasm compatibility #6785

johnpryan opened this issue Jun 21, 2023 · 29 comments

Comments

@johnpryan
Copy link

johnpryan commented Jun 21, 2023

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

@kevmoo
Copy link
Member

kevmoo commented Jun 21, 2023

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

@sigurdm sigurdm transferred this issue from dart-lang/pub Jun 23, 2023
@IchordeDionysos
Copy link

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?

@kevmoo
Copy link
Member

kevmoo commented Feb 15, 2024

@IchordeDionysos – yep!

@IchordeDionysos
Copy link

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.

@isoos
Copy link
Collaborator

isoos commented Feb 15, 2024

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.

@IchordeDionysos
Copy link

@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
Copy link

IchordeDionysos commented Feb 15, 2024

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

@isoos
Copy link
Collaborator

isoos commented Feb 15, 2024

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
Copy link
Collaborator

isoos commented Feb 15, 2024

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

@IchordeDionysos
Copy link

IchordeDionysos commented Feb 16, 2024

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

@amrgetment
Copy link

@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
flutter/flutter#117022

@amrgetment
Copy link

@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

@IchordeDionysos
Copy link

IchordeDionysos commented Feb 16, 2024

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

@kevmoo
Copy link
Member

kevmoo commented Feb 21, 2024

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

@jonasfj
Copy link
Member

jonasfj commented Jul 24, 2024

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
Copy link
Member

jonasfj commented Jul 24, 2024

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 🙈 (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.

@IchordeDionysos
Copy link

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.

@jonasfj
Copy link
Member

jonasfj commented Jul 25, 2024

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
Copy link
Member

jonasfj commented Jul 25, 2024

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.

@kevmoo
Copy link
Member

kevmoo commented Aug 13, 2024

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

@JaffaKetchup
Copy link

Not sure where the best place to report general false negatives are, maybe pana (eg. dart-lang/pana#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.

@kevmoo
Copy link
Member

kevmoo commented Sep 30, 2024

This should likely be copied to pkg:pana.

Thoughts @jonasfj ?

@jonasfj
Copy link
Member

jonasfj commented Sep 30, 2024

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

@JaffaKetchup
Copy link

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.

@jonasfj
Copy link
Member

jonasfj commented Sep 30, 2024

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.

@JaffaKetchup
Copy link

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

@josxha
Copy link

josxha commented Sep 30, 2024

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.

@sigurdm
Copy link
Contributor

sigurdm commented Sep 30, 2024

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
Copy link
Contributor

sigurdm commented Sep 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants