Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Turn @staticInterop tear-off into closure #41885

Merged
auto-submit merged 1 commit into from
May 11, 2023
Merged

Turn @staticInterop tear-off into closure #41885

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion lib/web_ui/lib/src/engine/text/font_collection.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,12 @@ class HtmlFontCollection implements FlutterFontCollection {
}

try {
fontFaces.forEach(domDocument.fonts!.add);
// Since we can't use tear-offs for interop members, this code is faster
// and easier to read with a for loop instead of forEach.
// ignore: prefer_foreach
Copy link
Contributor

@eyebrowsoffire eyebrowsoffire May 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid this lint override, you could instead do:

fontFaces.forEach((DomFontFace fontFace) => domDocument.fonts!.add(fontFace));

Copy link
Contributor Author

@srujzs srujzs May 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comes across another lint: https://dart.dev/tools/linter-rules#avoid_function_literals_in_foreach_calls :/

Copy link
Contributor

@eyebrowsoffire eyebrowsoffire May 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blech. Well then just ignore my comment for now. If we will eventually get tearoffs for static interop (you could even just have a kernel transform emit a closure like this for static interop tearoffs) maybe we should put a TODO here so that we can remove the lint override?

Copy link
Contributor

@eyebrowsoffire eyebrowsoffire May 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I read your more detailed comment, thanks for the context. I was imagining dynamic tearoff creation but I didn't think about tearoff equality... hmmm.

Copy link
Contributor

@eyebrowsoffire eyebrowsoffire May 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general these lint rules are awfully opinionated about things that don't seem to matter all that much 😆

Copy link
Contributor Author

@srujzs srujzs May 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, even if we were to ignore tearoff equality, the semantics is inconsistent between a regular call and a tear-off call, so in cases where optionality matters, users will still have to battle these lints.

I agree that there shouldn't be a lint for a regular for-loop, but that's my personal opinion. :)

for (final DomFontFace font in fontFaces) {
domDocument.fonts!.add(font);
}
} catch (e) {
return FontInvalidDataError(asset);
}
Expand Down