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

Detect and reward packages that are wasm compatible #1324

Open
kevmoo opened this issue Jan 31, 2024 · 27 comments
Open

Detect and reward packages that are wasm compatible #1324

kevmoo opened this issue Jan 31, 2024 · 27 comments
Labels
P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug

Comments

@kevmoo
Copy link
Member

kevmoo commented Jan 31, 2024

Repurposing this issue to track a specific platform/label of "good" wasm users. This will allow giving "bling" to folks on the package site.

@kevmoo kevmoo added type-enhancement A request for a change that isn't a bug P1 A high priority bug; for example, a single project is unusable or has many test failures labels Feb 1, 2024
@kevmoo
Copy link
Member Author

kevmoo commented Feb 1, 2024

Can link to http://dart.dev/go/package-web which will point to "the right docs" when we're ready

@kevmoo kevmoo moved this to Todo in Next-gen JS-interop Feb 6, 2024
@mit-mit
Copy link
Member

mit-mit commented Feb 6, 2024

It seems a bit premature to take points away before the old APIs are clearly marked as deprecated and new APIs are marked stable?

@kevmoo
Copy link
Member Author

kevmoo commented Feb 6, 2024

We've already added notes to the legacy APIs - https://api.dart.dev/main/dart-html/dart-html-library.html

We want a signal on pub first. Deprecations are loud and tend to cause lots of issues for the world. We just want to put (a bit) of pressure on the ecosystem.

We are not taking ANY points away. We're just giving the opportunity to earn more points for "doing the right thing" 😄

@kevmoo
Copy link
Member Author

kevmoo commented Feb 7, 2024

Had a great chat with @isoos and @jonasfj about this. Going to repurpose this request to track packages that are "wasm ready" as a label and then give them some "bling" on the package site.

@parlough
Copy link
Member

parlough commented Feb 7, 2024

Everyone will want some of the Wasm bling! I definitely like that more than reducing points as they can be a source of frustration for developers.

@kevmoo
Copy link
Member Author

kevmoo commented Feb 7, 2024

Like we could look into using the Wasm logo or something.

image

@sigurdm sigurdm changed the title Score down folks using legacy web APIs Detect and reward packages that are wasm compatible Feb 8, 2024
@mosuem
Copy link
Member

mosuem commented Mar 12, 2024

Is this still a P1 issue?

@kevmoo
Copy link
Member Author

kevmoo commented Mar 12, 2024

I'd like some of the false positives/negatives to be prioritized. maybe not p1, though.

@mosuem mosuem added P2 A bug or feature request we're likely to work on and removed P1 A high priority bug; for example, a single project is unusable or has many test failures labels Mar 13, 2024
@sigurdm
Copy link
Contributor

sigurdm commented Apr 2, 2024

@kevmoo, Can we try to flesh the remaining work out a bit.

What false positives/negatives do we know?

Currently a package gets is:wasm-ready iff:

  • all dart: libraries transitively reachable in the import graph from the main library belongs to the set
    • 'dart:async',
    • 'dart:collection',
    • 'dart:convert',
    • 'dart:core',
    • 'dart:developer',
    • 'dart:math',
    • 'dart:typed_data',
    • 'dart:_internal',
    • 'dart:ui',
    • 'dart:ui_web',
    • 'dart:js_interop',
    • 'dart:js_interop_unsafe'

Is that the right rule?

Do we have any other things we want to affect the decision?

@kevmoo
Copy link
Member Author

kevmoo commented Apr 2, 2024

@sigurdm – that sounds right, but we should look at the items at dart-lang/pub-dev#6785 (comment)

We should also exclude imports of pkg:js

@srujzs
Copy link

srujzs commented Apr 2, 2024

I don't know if allowlisting is easier vs denylisting, but from the interop side, anything from the "previous libraries" section here: https://dart.dev/interop/js-interop#next-generation-js-interop is considered not Wasm-compatible.

@sigurdm
Copy link
Contributor

sigurdm commented Apr 4, 2024

we should look at the items at dart-lang/pub-dev#6785 (comment)

Ah sorry - somehow overlooked this list:

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

This has migrated. I checked out 709bdcd4 to investigate.

The import is in lib/intl_browser.dart:12:import 'dart:html'; not the "main library" (named the same as the package) that is why we still assign the tag.

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

~/p/i18n_extension (master)> git grep 'package:intl'
lib/src/i18n_widget.dart:6:import 'package:intl/number_symbols.dart';
lib/src/i18n_widget.dart:7:import 'package:intl/number_symbols_data.dart';

It doesn't import the offending file from the intl package -> it gets the tag.

flutter_keyboard_visibility: false positive Depends on dart:html

Only depends on dart:html from:
https://github.com/MisterJimson/flutter_keyboard_visibility/blob/42ddef531e437cdd15b7baf270b64c8ae8ce8a28/flutter_keyboard_visibility_web/lib/flutter_keyboard_visibility_web.dart#L4

Not from the main library file (the one named the same as the library).

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

No longer true.

I checked out the last version containing the imports 6af2a0c8, and I got the following tags:

 "tags": [
  "sdk:flutter",
  "platform:web",
  "is:plugin",
  "is:null-safe",
  "is:dart3-compatible",
  "license:mit",
  "license:fsf-libre",
  "license:osi-approved"
 ],

Seems OK

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

This one is interesting. The flutter_secure_storage_web package is a federated plugin. I believe their import relies on code-generation (@jonasfj is this true?) I don't know how we would handle that.

Also the federated plugin structure means that you could potentially replace flutter_secure_storage_web with another package that was not importing package:js. So is this really a strict positive?

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

Another interesting one: https://github.com/fluttercommunity/plus_plugins/blob/main/packages/sensors_plus/sensors_plus/lib/sensors_plus.dart#L7 has a conditional import of the web-part. But that is conditioned on dart:html.

When compiling for WASM the (dart:html) condition will be false, and thus the file will not be imported. Thus the package is broken on WASM, but we don't detect it.

To me that shows a weakness of the conditional import system.

Not sure how we should handle this.

uni_links: false positive: Depends on dart:html

Not the main lib.

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

Indeed. The we index the latest stable (non-prerelease) version.

We should also exclude imports of pkg:js

Those are already excluded (that is the second 'f' of iff).

@mosuem
Copy link
Member

mosuem commented Apr 4, 2024

intl: false positive

package:intl is WIP. I will try to reland the switch to WASM soon. See dart-lang/i18n#822

@sigurdm
Copy link
Contributor

sigurdm commented Apr 29, 2024

It indeed seems detecting wasm-incompatibility for federated plugins will be hard. Flutter generates code that imports the platform specific implementations -> we cannot just follow import paths.

https://github.com/flutter/flutter/blob/1a0dc8f1e11892f8f2365ffb6a9617abc39d4b15/packages/flutter_tools/lib/src/build_system/targets/dart_plugin_registrant.dart#L30

But one could argue that a federated plugin is wasm-ready (while it's default web option might not be). But that is of course mostly being technically correct...

@isoos
Copy link
Collaborator

isoos commented Apr 29, 2024

But that is of course mostly being technically correct...

Note: a similar case happened with my docker_process package: somebody wanted to know how it runs on Android (and iOS). Technically it executes the docker executable, but in practice it won't do it, as it won't usually run on mobile phones. I am a bit undecided if at all we could detect that, or should we just suggest the package author to fill out platforms: in the pubspec. If the later, we should have a similar entry for wasm maybe?

@Rexios80
Copy link
Contributor

WASM support is stable with Flutter 3.22.0. I would have expected this to be ready for the release or even well before so developers know what packages work with it and package maintainers get nudged to update. Has there been any progress?

@isoos
Copy link
Collaborator

isoos commented May 18, 2024

@Rexios80: Have you checked this search filter on pub.dev?

image

@Rexios80
Copy link
Contributor

That is not very discoverable, there isn't a badge on compatible packages, and it doesn't do anything to incentivize package maintainers to support WASM. I think adding another 10 points for WASM support would be much preferable as that is a strong nudge to migrate. Are there any APIs dart:html has that are not supported with the new web interop libraries? If there aren't I see no reason to avoid docking points on packages that haven't migrated.

@jonasfj
Copy link
Member

jonasfj commented May 18, 2024

Currently pub.dev has badges for:

  • SDK: Flutter | Dart
  • Platform: Windows | Android | ... | Web

We don't have badges, but we do have search filters (if you read help pages) for:

  • Runtime: AOT | JIT | JS

I think wasm is more like a runtime than a platform. Hence, I don't think we know how we want to add it in the UI.

We could add a different kind of badge: wasm-ready, like we had for null-safety, before it became the norm.
Maybe, that's a path. It's just we'll be spamming all the pure-Dart packages that don't interface js/html with this badge too.


I think granting 10 points or something like that for packages that are wasm-ready, in the platform scoring section might be reasonable.

Not sure was @kevmoo thinks about all this.

@kevmoo
Copy link
Member Author

kevmoo commented May 19, 2024

The thing that makes most sense is flagging legacy-html honestly.
And giving 5-10 points for packages that AVOID legacy-html imports.

That's my technical answer, but I'd want to brainstorm the right "community/ecosystem" way to do this so we don't hurt folks feelings.

@jonasfj
Copy link
Member

jonasfj commented May 21, 2024

The thing that makes most sense is flagging legacy-html honestly.

I think we should be careful flagging it just yet.
I mean it'll be a LONG time before dart:html goes away, right? (if ever)


But I think that giving 5-10 points for avoiding dart:html and friends is a great solution.

And giving 5-10 points for packages that AVOID legacy-html imports.

That probably includes avoiding transitive imports of legacy-html.

@Rexios80
Copy link
Contributor

Is there a reason for dart:html to stick around besides maintainers not migrating? I see no reason to not penalize them for not keeping up to date with Dart best practices.

@jonasfj
Copy link
Member

jonasfj commented May 21, 2024

Is there a reason for dart:html to stick around besides maintainers not migrating?

I think it's safe to assume that not all migrations are trivial.

There are some very large Dart code bases out there. Migrating them might take years. It might be easier to rely on backwards compatibility, even if you don't get some of the benefits of the new interop model.

@isoos
Copy link
Collaborator

isoos commented May 21, 2024

Is there a reason for dart:html to stick around besides maintainers not migrating? I see no reason to not penalize them for not keeping up to date with Dart best practices.

It is worth to keep in mind that life happens to all of us, and not everyone is ready to put effort in migrating on day one (or day 20). If you need a package to be migrated, please, by all means reach out to its developers, and help them to do so. API deprecation and with that some kind of penalty may happen down the line, but I don't see any reason to rush it.

@Rexios80
Copy link
Contributor

I don't see any reason to not rush it. WASM support is touted as "stable" in Flutter 3.22.0 and yet six out of seven of my web projects fail to build due to various migration issues that are outside of my control. I'm afraid if we don't push hard for migration now it just won't happen for many packages.

@Rexios80
Copy link
Contributor

Also how is this any different than regular deprecations in the Dart/Flutter SDKs. Those already immediately cause loss of points due to the analysis issues they generate.

@isoos
Copy link
Collaborator

isoos commented May 21, 2024

@Rexios80: In my experience SDK deprecations rarely affect the score of a well-maintained package. wasm support is a new feature, less than a week old in a stable SDK. The effect and scale is not even comparable.

Again: if it is urgent for you for you, please consider to help out the developers of the packages that have a migration task ahead of them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 A bug or feature request we're likely to work on type-enhancement A request for a change that isn't a bug
Projects
Status: Todo
Development

No branches or pull requests

9 participants