dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.3k stars 1.59k forks source link

Don't trigger `avoid_web_libraries_in_flutter` in files ending with `_web.dart` #59381

Open parlough opened 10 months ago

parlough commented 10 months ago

Currently avoid_web_libraries_in_flutter is not triggered if a Flutter web plugin context is added. However this mechanism doesn't fit all uses of web libraries.

As an example, various DevTools' packages use a lot of web libraries without this declaration, resulting in them adding a lot of ignore comments. However, a majority of these files have a name ending with _web.dart.

I think that's a pretty clear opt-in to wanting access to the underlying web platform. So I propose we don't trigger the lint in that case, and suggest it as a standard naming scheme for files that access the web platform in a multi-platform Flutter app.

This will be particularly helpful as I'd like to expand the lint to include the new replacement web libraries, but some code has been migrated to them without the ignore. This will reduce new cases of the lint triggering.

Draft implementation CL: https://dart-review.googlesource.com/c/sdk/+/349180

bwilkerson commented 10 months ago

@goderbauer @kevmoo As potentially interested parties.

goderbauer commented 10 months ago

I am not that familiar with how and when web libraries should be used in Flutter. Maybe @yjbanov can comment?

kevmoo commented 10 months ago

_web.dart is a nice convention. I can buy it. It's hard to imagine a case where someone wants to name a library XYZ_web.dart and not want it to be "web stuff".

kevmoo commented 10 months ago

These kinds of magic conventions, in general, make me nervous though. I guess if we document it clearly in the lint?

parlough commented 10 months ago

These kinds of magic conventions, in general, make me nervous though.

Yeah I was hoping this one was more clear than some other magic conventions with it involving the filename, which is generally quite visible. Likely even more so than the pubspec's plugin configuration which can be quite far away from a developer's current context, or modified by a different developer. Hopefully updated documentation can help too :)

I guess if we document it clearly in the lint?

I agree! You can see my proposed documentation in https://dart-review.googlesource.com/c/sdk/+/349180.

parlough commented 9 months ago

After some synchronous discussion, there's feeling that accidental imports of dart:html when writing Flutter code was perhaps the primary motivator for this lint. Now that dart:html is not suggested if not imported already, this lint might not be as necessary.

There's also the fact that it's alone in avoiding platform handling, as there is no equivalent lint for dart:io and similar.

I'd still like to hear what the Flutter web team thinks, but I'm leaning towards not expanding this lint to cover the new libraries (dart:js_interop, dart:js_interop_unsafe, etc), nor adding new mechanisms to avoid it.

Then in a world without dart:html, we can deprecate this lint and remove it from package:flutter_lints.

There is still value in helping developers use conditional imports correctly and isolating platform-specific code, but any solution should likely be designed from the bottom-up to account for all platforms, not just web.

Let me know your thoughts :)

bwilkerson commented 9 months ago

@yjbanov Friendly ping. Your opinion would be welcome here.

kevmoo commented 9 months ago

@bwilkerson – I think we're okay to move forward here!

kevmoo commented 9 months ago

Although we should talk about what's considered a web library with the new things coming along. Maybe that should be a new issue...

bwilkerson commented 9 months ago

I'm ok with changing the lint, but I do want to have a clear definition of the semantics we should implement before we start. And I want to know what the end-state looks like, even if we're expecting to be in this intermediate state for long enough to make supporting it valuable to users. Happy to get there in whatever way works best for you.