[flutter_svg] wasm compatibility #8014

nank1ro · 2024-11-04T09:40:26Z

Make flutter_svg wasm compatible

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2024-11-04T09:40:30Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

stuartmorgan

Thanks for the contribution!

stuartmorgan · 2024-11-04T12:54:29Z

packages/vector_graphics_compiler/pubspec.yaml

@@ -36,7 +36,7 @@ dev_dependencies:
    sdk: flutter
  test: ^1.20.1
  # See https://github.com/flutter/flutter/issues/157626
-  vector_graphics: ">=1.1.11+1 <= 1.1.12"
+  vector_graphics: ">=1.1.11+1 <= 1.1.14"


1.1.14 doesn't exist yet, so we don't know if it'll be compatible under the versioning system vector_graphics has used previously.

Not on pub.dev, but I just bumped it here https://github.com/flutter/packages/pull/8014/files#diff-ae3c1055bc547ec174edea702c972431d2d3d46fb9bd78a31378cacf0fd051eaR8

If I don't bump the constraints here, how is it supposed to work?

Sorry, I missed that you were changing vector_graphics, because #7991 already did vector_graphics, and the PR title says this is flutter_svg, not vector_graphics. If vector_graphics* still needs changes, those should be a separate PR.

You're right. Didn't notice someone already made vector_graphics wasm compatible. I just fixed the dev_dependency constraint to use the minimum the WASM version. Do you prefer another PR for a single line change (just dev_dependency)?

As long as the changes aren't interdependent, they can be in a single PR. If there are still any cases where you need to both change a package, and change the minimum dependency on that package in another package, that must be multiple PRs because it won't compile in CI.

If you're only changing a dev dependency in v_g_c, it should be fine to include here. But that would also mean you can't have (and shouldn't need) this change in the PR.

stuartmorgan · 2024-11-04T12:54:59Z

packages/vector_graphics/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 1.1.14
+
+* Make the package WASM compatible


Please see the CHANGELOG style guide linked from the PR checklist; this needs to use the repo-standard verb form and punctuation.

stuartmorgan · 2024-11-04T12:55:51Z

packages/vector_graphics_compiler/pubspec.yaml

@@ -36,7 +36,7 @@ dev_dependencies:
    sdk: flutter
  test: ^1.20.1
  # See https://github.com/flutter/flutter/issues/157626
-  vector_graphics: ">=1.1.11+1 <= 1.1.12"
+  vector_graphics: ">=1.1.11+1 <= 1.1.14"


Isn't 1.1.13 the minimum required for wasm compatibility?

vector_graphics was already at 1.1.13, I bumped it to 1.1.14 but the v1.1.13 was not used here, no idea why

You bumped the maximum, not the minimum. The CHANGELOG entry says this PR makes the package wasm compatible, which would generally involve also setting the minimum versions of any dependencies to wasm-compatible versions.

You're right, updated it right now

stuartmorgan · 2024-11-04T12:56:27Z

packages/vector_graphics/pubspec.yaml

@@ -22,7 +22,7 @@ dev_dependencies:
  flutter_test:
    sdk: flutter
  # See https://github.com/flutter/flutter/issues/157626
-  vector_graphics_compiler: ">=1.1.11+1 <= 1.1.12"
+  vector_graphics_compiler: ">=1.1.11+1 <= 1.1.13"


1.1.13 doesn't exist; what's the purpose of this change?

the same as #8014 (comment)

stuartmorgan · 2024-11-04T12:56:37Z

third_party/packages/flutter_svg/pubspec.yaml

  vector_graphics_codec: ">=1.1.11+1 <= 1.1.12"
-  vector_graphics_compiler: ">=1.1.11+1 <= 1.1.12"
+  vector_graphics_compiler: ">=1.1.11+1 <= 1.1.13"


Same question here as in the other package.

the same as #8014 (comment)

stuartmorgan · 2024-11-04T12:56:44Z

third_party/packages/flutter_svg/pubspec.yaml

@@ -13,9 +13,9 @@ dependencies:
    sdk: flutter
  http: ^1.0.0
  # See https://github.com/flutter/flutter/issues/157626
-  vector_graphics: ">=1.1.11+1 <= 1.1.12"
+  vector_graphics: ">=1.1.11+1 <= 1.1.14"


Same notes here as in the other package.

the same as #8014 (comment)

stuartmorgan · 2024-11-04T12:58:12Z

test-exempt: configuration change, rolling dependencies

stuartmorgan · 2024-11-04T13:01:51Z

test-exempt: configuration change, rolling dependencies

Actually, wait. @ditman Why didn't I have to add these packages to exclude_all_packages_app_wasm.yaml when I imported them? The test coverage here should be that we opt it into the wasm build. Are we not testing wasm compilation correctly?

(This isn't a blocker for landing this PR, but we should figure out what's going on with our test coverage here.)

stuartmorgan · 2024-11-04T15:38:19Z

packages/vector_graphics/lib/src/debug.dart

@@ -2,4 +2,4 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

-export '_debug_web.dart' if (dart.library.io) '_debug_io.dart';


This code was changed for wasm support in #7991; why is it being re-changed here?

Because I didn't now this one works too, I always used the inverse with dart.library.js_interop to be consistent

stuartmorgan · 2024-11-04T15:39:08Z

packages/vector_graphics_compiler/lib/vector_graphics_compiler.dart

@@ -18,9 +18,9 @@ import 'src/svg/theme.dart';
 import 'src/vector_instructions.dart';

 export 'src/_initialize_path_ops_io.dart'
-    if (dart.library.html) 'src/_initialize_path_ops_web.dart';
+    if (dart.library.js_interop) 'src/_initialize_path_ops_web.dart';


We should ideally be consistent about how we switch on web compilation. @kevmoo / @ditman is there a preference between this and io (as was done in #7991)?

io is probably more "permanent" a library, for web conditional imports js_interop has changed in the past (used to be html).

js_interop however is going to be with us for a few years, so it probably doesn't matter much which conditional import to do.

(It feels a little bit weird to export "web" first though, but that's just me because I've almost always started from a mobile implementation!)

nank1ro · 2024-11-05T09:40:47Z

@stuartmorgan I opened another PR for the package vector_graphics_compiler.
The other PR must be merged first and the package published on pub dev so this PR can pass all the tests.
I made all the changes requested, let me know if there is something else I can do.

stuartmorgan · 2024-11-05T12:57:34Z

The other PR must be merged first and the package published on pub dev so this PR can pass all the tests.

Why? Per this comment I'm not clear why you are updating the minimum version in the dependencies on vector_graphics_compiler when the only change you are making in vector_graphics_compiler is a dev_dependency change. By definition, a dev_dependency change doesn't impact transitive dependencies.

nank1ro · 2024-11-05T13:52:53Z

The other PR must be merged first and the package published on pub dev so this PR can pass all the tests.

Why? Per this comment I'm not clear why you are updating the minimum version in the dependencies on vector_graphics_compiler when the only change you are making in vector_graphics_compiler is a dev_dependency change. By definition, a dev_dependency change doesn't impact transitive dependencies.

Because vector_graphics_compiler is in the dependencies of flutter_svg, see here.
To make flutter_svg wasm compatibile, all dependencies must be wasm compatible

stuartmorgan · 2024-11-05T15:42:53Z

Oh, I see now. I thought the vector_graphics_compiler changes were dev-only; I missed that the source changes are in non-test code.

nank1ro · 2024-11-06T11:28:18Z

@stuartmorgan I fixed the conflics and merged main. If the tests pass I think it's ready to be merged

stuartmorgan · 2024-11-06T13:00:29Z

third_party/packages/flutter_svg/pubspec.yaml

  vector_graphics_codec: ^1.1.11+1
-  vector_graphics_compiler: ^1.1.11+1
+  vector_graphics_compiler: ^1.1.13


Based on the order things landed, this should be 1.1.14 (vector_graphics and vector_graphics_compiler ended up with opposite order of WASM support vs relaxed dependencies, it looks like.)

you're right, fixed

stuartmorgan

LGTM. Thanks for getting this submitted to the new repository so quickly, and for bearing with the churn while the dust was still settling on the initial import!

stuartmorgan · 2024-11-06T14:33:04Z

@jonahwilliams / @domesticmouse / @ditman Could one of you do the secondary approval?

kevmoo · 2024-11-06T20:47:43Z

third_party/packages/flutter_svg/lib/src/utilities/file.dart

@@ -1 +1 @@
-export '_file_io.dart' if (dart.library.html) '_file_none.dart';
+export '_file_io.dart' if (dart.library.js_interop) '_file_none.dart';


Sadly, this might still rank this package as NOT wasm-compatible in pub due to issues with configuration specific imports.

Better to do

Suggested change

export '_file_io.dart' if (dart.library.js_interop) '_file_none.dart';

export '_file_none.dart' if (dart.library.io) '_file_io.dart';

Other PRs that this one relies on have already landed using this version since #8014 (comment) didn't raise any objection.

Could you elaborate on the issue?

No I can confirm this works because I did it with the fork I published, flutter_svg_plus

You can check the platform support inside the score here https://pub.dev/packages/flutter_svg_plus/score

I have no doubt that it works at runtime. I'm concerned that the wasm classification on pub.dev will still be broken without this change.

See dart-lang/pana#1324

See https://pub.dev/api/packages/flutter_svg/metrics

Search for package:flutter_svg/src/utilities/file.dart

If you think the score for wasm on pub.dev is wrong, I assure you it is not.
See here https://pub.dev/api/packages/flutter_svg_plus/metrics and here https://pub.dev/api/packages/vector_graphics_compiler/metrics.
The latter is the one which has been already merged using dart.library.js_interop as a conditional import

See dart-lang/pana#1324

I'm confused; I don't see anything in that issue that says that a conditional import based on js_interop wouldn't work, and it's currently working in practice. Could you point to a specific discussion/comment that makes you think this won't work in the future?

See https://pub.dev/api/packages/flutter_svg/metrics

Search for package:flutter_svg/src/utilities/file.dart

Why are we looking at the published version of flutter_svg, which doesn't have the fix in this PR and still uses dart:html?

stuartmorgan · 2024-11-07T12:32:53Z

Given that the published version using dart.library.html definitely doesn't work, dart.library.js_interop is currently working with pana, and we have already published other libraries using it, I'm going to go ahead and land this. If we get confirmation later that this will stop working for some reason, we will already need to do a follow-up PR for other packages in the dependency chain anyway, so we can just update this at the same time.

* main: (1187 commits) [various] Update example app minSdkVersions (flutter#8035) [go_router] Activate leak testing (flutter#7546) [in_app_purchase_storekit] Add restore purchases and receipts (flutter#7964) [interactive_media_ads] Adds remaining methods for internal wrapper of the Android native `BaseManager` (flutter#7943) [google_sign_in/google_identity_services] Clear-up documentation of callbacks in various APIs and uses of those APIs (flutter#8029) [flutter_svg] wasm compatibility (flutter#8014) Applied Gradle Plugins Declaratively for Multiple Plugin Example Apps (Part 2) (flutter#8019) Roll Flutter from 29d40f7f6826 to 73546b3b71a7 (20 revisions) (flutter#8028) [ci] Upload screenshots, logs, and Xcode test results for drive and integration_test runs (flutter#7430) Remove use_modular_headers! from Podfiles (flutter#7796) [camera_avfoundation] enable more than 30 fps (flutter#7394) Roll Flutter from 8591d0c16a6c to 29d40f7f6826 (25 revisions) (flutter#8027) [ci] Add vector_graphics and flutter_svg to autolabeler (flutter#8025) [vector_graphics_compiler] wasm compatibility (flutter#8021) [vector_graphics*] Relax dependency constraints of vector_graphics, vector_graphics_codec, vector_graphics_compiler, flutter_svg (flutter#8018) [various] Add `missing_code_block_language_in_doc_comment` lint to flutter/packages. (flutter#6473) [various] Update example apps to Kotlin 1.9.0 (flutter#7998) [go_router] add current state getter (flutter#7651) Applied Gradle Plugins Declaratively for Multiple Plugin Example Apps (flutter#7968) Roll Flutter from f86b77721524 to 8591d0c16a6c (16 revisions) (flutter#8015) ... # Conflicts: # packages/quick_actions/quick_actions/CHANGELOG.md # packages/quick_actions/quick_actions_ios/CHANGELOG.md # packages/quick_actions/quick_actions_platform_interface/CHANGELOG.md

flutter/packages@bb5a258...72356fd 2024-11-08 stuartmorgan@google.com [various] Update example app minSdkVersions (flutter/packages#8035) 2024-11-07 32538273+ValentinVignal@users.noreply.github.com [go_router] Activate leak testing (flutter/packages#7546) 2024-11-07 louisehsu@google.com [in_app_purchase_storekit] Add restore purchases and receipts (flutter/packages#7964) 2024-11-07 10687576+bparrishMines@users.noreply.github.com [interactive_media_ads] Adds remaining methods for internal wrapper of the Android native `BaseManager` (flutter/packages#7943) 2024-11-07 58529443+srujzs@users.noreply.github.com [google_sign_in/google_identity_services] Clear-up documentation of callbacks in various APIs and uses of those APIs (flutter/packages#8029) 2024-11-07 alex@mariuti.com [flutter_svg] wasm compatibility (flutter/packages#8014) 2024-11-06 jessiewong401@gmail.com Applied Gradle Plugins Declaratively for Multiple Plugin Example Apps (Part 2) (flutter/packages#8019) 2024-11-06 engine-flutter-autoroll@skia.org Roll Flutter from 29d40f7 to 73546b3 (20 revisions) (flutter/packages#8028) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

nank1ro added 2 commits November 4, 2024 10:31

refactor/wasm-compatibility

6c3cc75

chore/bump versions

7b397a4

nank1ro requested review from domesticmouse and jonahwilliams as code owners November 4, 2024 09:40

stuartmorgan requested changes Nov 4, 2024

View reviewed changes

chore/update changelog message

6a62471

stuartmorgan reviewed Nov 4, 2024

View reviewed changes

nank1ro added 7 commits November 4, 2024 16:55

Merge branch 'main' into refactor/wasm-compatibility

d857895

refactor/revert changes in vector_graphics

5784d44

chore: update constraints of dev dependencies

4c522e1

Merge branch 'main' into refactor/wasm-compatibility

435719f

refactor: revert changes in vector graphics compiler

d2cfe33

fix: min vector graphics version

2f2e450

fix compiler min version

bcfe1ea

nank1ro added 2 commits November 5, 2024 10:43

Merge branch 'main' into refactor/wasm-compatibility

3721055

bump version

6029722

Merge branch 'main' into refactor/wasm-compatibility

32fbf69

github-actions bot added the p: flutter_svg label Nov 6, 2024

stuartmorgan reviewed Nov 6, 2024

View reviewed changes

Update pubspec.yaml

0ca88ce

stuartmorgan approved these changes Nov 6, 2024

View reviewed changes

zanderso approved these changes Nov 6, 2024

View reviewed changes

kevmoo reviewed Nov 6, 2024

View reviewed changes

stuartmorgan added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 7, 2024

auto-submit bot merged commit da51281 into flutter:main Nov 7, 2024
76 checks passed

nank1ro deleted the refactor/wasm-compatibility branch November 7, 2024 14:23

engine-flutter-autoroll mentioned this pull request Nov 7, 2024

Roll Packages from bb5a25815cae to da51281bbc3e (3 revisions) flutter/flutter#158322

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 7, 2024

da51281bb [flutter_svg] wasm compatibility (flutter/packages#8014)

2b793f7

engine-flutter-autoroll mentioned this pull request Nov 7, 2024

Roll Packages from bb5a25815cae to 4b2132f394c6 (4 revisions) flutter/flutter#158334

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 7, 2024

da51281bb [flutter_svg] wasm compatibility (flutter/packages#8014)

5d428b8

engine-flutter-autoroll mentioned this pull request Nov 8, 2024

Roll Packages from bb5a25815cae to 72356fda4684 (8 revisions) flutter/flutter#158378

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 8, 2024

da51281bb [flutter_svg] wasm compatibility (flutter/packages#8014)

d0e1892

engine-flutter-autoroll mentioned this pull request Nov 11, 2024

Roll Packages from bb5a25815cae to d681e4e404e8 (15 revisions) flutter/flutter#158468

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 11, 2024

da51281bb [flutter_svg] wasm compatibility (flutter/packages#8014)

15a96c5

		@@ -1 +1 @@
		export '_file_io.dart' if (dart.library.html) '_file_none.dart';
		export '_file_io.dart' if (dart.library.js_interop) '_file_none.dart';

	export '_file_io.dart' if (dart.library.js_interop) '_file_none.dart';
	export '_file_none.dart' if (dart.library.io) '_file_io.dart';

[flutter_svg] wasm compatibility #8014

[flutter_svg] wasm compatibility #8014

Conversation

nank1ro commented Nov 4, 2024 • edited by stuartmorgan Loading

Pre-launch Checklist

flutter-dashboard bot commented Nov 4, 2024

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan commented Nov 4, 2024

stuartmorgan commented Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nank1ro commented Nov 5, 2024

stuartmorgan commented Nov 5, 2024 • edited Loading

nank1ro commented Nov 5, 2024 • edited Loading

stuartmorgan commented Nov 5, 2024

nank1ro commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

stuartmorgan commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nank1ro Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nank1ro Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

stuartmorgan Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

stuartmorgan commented Nov 7, 2024

nank1ro commented Nov 4, 2024 •

edited by stuartmorgan

Loading

stuartmorgan Nov 4, 2024 •

edited

Loading

stuartmorgan commented Nov 4, 2024 •

edited

Loading

stuartmorgan commented Nov 5, 2024 •

edited

Loading

nank1ro commented Nov 5, 2024 •

edited

Loading

nank1ro Nov 6, 2024 •

edited

Loading

nank1ro Nov 7, 2024 •

edited

Loading

stuartmorgan Nov 7, 2024 •

edited

Loading