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

[google_maps_flutter_web] Migrate to package:web #5254

Merged
merged 43 commits into from
Feb 28, 2024

Conversation

navaronbracke
Copy link
Contributor

@navaronbracke navaronbracke commented Oct 28, 2023

This PR migrates google_maps_flutter_web from dart:html to package:web.

The google_maps package does provide a beta version with support for package:web, which is currently what this PR is depending on. Before landing this change, we should change the dependency to the stable release version of that package.

Fixes flutter/flutter#137374

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
N/A

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

treeSanitizer: NodeTreeSanitizer.trusted,
);
container.children.add(snippet);
..innerHTML = sanitizeHtml(markerSnippet);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no NodeSanitizer anymore, not sure of the implications.

Copy link
Member

@ditman ditman Feb 14, 2024

Choose a reason for hiding this comment

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

I think the "safe" way to solve this now is to :

Then define a setter for HTMLElement that lets us set the innerHTML property with a TrustedHTML instance, rather than the current Node or HTMLElement type.

The sanitizeHtml method could be the implementation of the Trusted Type policy, as described in the MDN example:

const escapeHTMLPolicy = trustedTypes.createPolicy("flutter-sanitize-maps", {
  createHTML: (String html) => sanitizeHtml(html), // Returns an instance of TrustedHTML
});

let el = document.getElementById("myDiv");
const escaped = escapeHTMLPolicy.createHTML(markerSnippet);
el.safeInnerHTML = escaped; // safeInnerHTML only accepts TrustedHTML objects.

(The above is a js-ification of what we need to do in Dart though :P)

Copy link
Contributor Author

@navaronbracke navaronbracke Feb 14, 2024

Choose a reason for hiding this comment

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

The support for TrustedHTML is a bit lacking though, neither Safari nor Firefox support it.

Copy link
Member

@ditman ditman Feb 27, 2024

Choose a reason for hiding this comment

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

neither Safari nor Firefox support it.

The stance until very recently was that they'd never will support it, but that's what the unsafe_html lint is actually looking at 😱

Also Mozilla changed their minds recently!

@ditman
Copy link
Member

ditman commented Jan 10, 2024

There's progress of the dart-google-maps package migration, here:

a14n/dart-google-maps#122 (comment)

@navaronbracke
Copy link
Contributor Author

navaronbracke commented Jan 11, 2024

I've updated the branch to use the new beta version of google_maps, and made some last adjustments.
Next, I'll have to run the tests to see if any fail. Once the tests pass, we can adjust the version of google_maps and get this into a reviewable state.

Edit: Still have to help the web compiler out a little, still have JS type violations to fix.

@navaronbracke
Copy link
Contributor Author

navaronbracke commented Jan 11, 2024

The google_maps_flutter_web integration tests now pass locally, but I do see a different error in the console still?

Logs
js_primitives.dart:30 ══╡ EXCEPTION CAUGHT BY GESTURES LIBRARY ╞══════════════════════════════════════════════════════════
js_primitives.dart:30 The following StateError was thrown while handling a pointer data packet:
js_primitives.dart:30 Bad state: No element
js_primitives.dart:30 
js_primitives.dart:30 When the exception was thrown, this was the stack:
js_primitives.dart:30 dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/errors.dart 297:3       throw_
js_primitives.dart:30 dart-sdk/lib/core/iterable.dart 702:5                                             firstWhere
js_primitives.dart:30 packages/flutter_test/src/test_text_input_key_handler.dart.js 11893:63            handlePointerEvent
js_primitives.dart:30 packages/flutter/src/gestures/binding.dart.js 370:14                              [_flushPointerEventQueue]
js_primitives.dart:30 packages/flutter/src/gestures/binding.dart.js 345:40                              [_handlePointerDataPacket]
js_primitives.dart:30 lib/_engine/engine/platform_dispatcher.dart 1361:5                                invoke1
js_primitives.dart:30 lib/_engine/engine/platform_dispatcher.dart 286:5                                 invokeOnPointerDataPacket
js_primitives.dart:30 lib/_engine/engine/pointer_binding.dart 398:30                                    [_sendToFramework]
js_primitives.dart:30 lib/_engine/engine/pointer_binding.dart 224:7                                     onPointerData
js_primitives.dart:30 lib/_engine/engine/pointer_binding.dart 978:16                                    <fn>
js_primitives.dart:30 lib/_engine/engine/pointer_binding.dart 930:7                                     <fn>
js_primitives.dart:30 lib/_engine/engine/pointer_binding.dart 531:9                                     loggedHandler
js_primitives.dart:30 dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart 550:37  _checkAndCall
js_primitives.dart:30 dart-sdk/lib/_internal/js_dev_runtime/private/ddc_runtime/operations.dart 555:39  dcall
js_primitives.dart:30 ════════════════════════════════════════════════════════════════════════════════════════════════════
js_primitives.dart:30 00:00 +0: TileOverlaysController addTileOverlays
js_primitives.dart:30 00:00 +1: TileOverlaysController changeTileOverlays
js_primitives.dart:30 00:00 +2: TileOverlaysController updating the z index of a hidden layer does not make it visible
js_primitives.dart:30 00:00 +3: TileOverlaysController removeTileOverlays
js_primitives.dart:30 00:00 +4: TileOverlaysController clearTileCache
js_primitives.dart:30 00:01 +5: (tearDownAll)
js_primitives.dart:30 00:01 +6: All tests passed!
js_primitives.dart:30 No widgets found at Offset(1448.7, 217.7).

Some CI tasks still fail because of a version problem with package:web:

Running command: "flutter pub get" in ./all_packages
Resolving dependencies...
Note: web is pinned to version 0.3.0 by flutter_test from the flutter SDK.
See https://dart.dev/go/sdk-version-pinning for details.


Because every version of google_maps_flutter_web from path depends on web ^0.4.0 and every version of flutter_test from sdk depends on web 0.3.0, google_maps_flutter_web from path is incompatible with flutter_test from sdk.
So, because all_packages depends on both flutter_test from sdk and google_maps_flutter_web from path, version solving failed.
Failed to generate native build files via 'flutter pub get'

The new google_maps version requires package:web 0.4.0, so I guess we wait until the packages roller picks it up?

@navaronbracke navaronbracke changed the title [WIP] [google_maps_flutter_web] Migrate to package:web [google_maps_flutter_web] Migrate to package:web Jan 11, 2024
@navaronbracke navaronbracke marked this pull request as ready for review January 11, 2024 10:51
@@ -20,6 +20,9 @@ class MyApp extends StatefulWidget {
class _MyAppState extends State<MyApp> {
@override
Widget build(BuildContext context) {
return const Text('Testing... Look at the console output for results!');
return const Directionality(
Copy link
Contributor Author

@navaronbracke navaronbracke Jan 23, 2024

Choose a reason for hiding this comment

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

I accidentally ran the example app once directly (using flutter run -d chrome) and it did throw an error about a missing Directionality widget

Copy link
Member

Choose a reason for hiding this comment

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

This is fine, the skeleton app is only used to have a Flutter context where things run, but IIRC this particular test does not render a map or anything.

@navaronbracke
Copy link
Contributor Author

I have three failing integration tests, which all relate to the bitmap of the test Tile. Not sure what the resolution is? (this refactor should not have affected the loading of the bitmap)

  • The markers with custom bitmap icon work test in markers_test.dart fails because the list of bytes does not match the expected list
  • The produces image tiles test in overlay_test.dart fails due to the image element in the test triggering its onerror handler, even though we only try to load a transparent 16x16 bitmap which we don't get over the network (we decode it ourselves)?
  • The update test in overlay_test.dart fails the same way as the produces image tiles test

@ditman
Copy link
Member

ditman commented Jan 23, 2024

We have a thread in discord about the test failures here:

@navaronbracke
Copy link
Contributor Author

navaronbracke commented Jan 31, 2024

@ditman The test failure was resolved on my end. Turns out you were right about JS converting stuff to strings, as mentioned in dart-lang/web#91 (comment) See 44e4335 for that fix.

@ditman
Copy link
Member

ditman commented Feb 14, 2024

@navaronbracke thanks for keeping this PR fresh, if you don't mind I may be able to push stuff to this branch to fix the annoying bits of CI?

@navaronbracke
Copy link
Contributor Author

Feel free to do so, I'll give it some attention regarding your review comments as well.

@navaronbracke
Copy link
Contributor Author

navaronbracke commented Feb 19, 2024

@ditman What version bump should this be for google_maps_flutter_web ? I have currently set it to version 0.5.5, as the version is still pre-1.0. However, it would force users to upgrade to the latest stable? Do we pick 0.6.0 instead then?

@navaronbracke
Copy link
Contributor Author

navaronbracke commented Feb 22, 2024

@kevmoo With package:web 0.5.0 the TrustedTypes binding was removed. However, I needed it for a small part here.

Could you look into landing dart-lang/web@975e55c officially as 0.5.1 ?

@kevmoo
Copy link
Contributor

kevmoo commented Feb 22, 2024

@navaronbracke – you can always just add your own extensions or helpers, too. Copy-paste here is fine.

@navaronbracke
Copy link
Contributor Author

@navaronbracke – you can always just add your own extensions or helpers, too. Copy-paste here is fine.

I'm aware of that, I did this for the nullable TrustedTypes in this PR. However, the TrustedTypes definition was removed between 0.4.2 and 0.5.0. I guess I'll just copy over the definition.

@ditman
Copy link
Member

ditman commented Feb 28, 2024

This seems to work fine, and I can't find anything else to say! Let's land this! Thanks for your patience @navaronbracke!

Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

Let's go!

@ditman ditman added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 28, 2024
@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 28, 2024

This comment was marked as outdated.

@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 28, 2024
@auto-submit auto-submit bot merged commit 7cdcf30 into flutter:main Feb 28, 2024
78 checks passed
@navaronbracke navaronbracke deleted the google_maps_web_pkg_web branch February 28, 2024 07:10
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 29, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 29, 2024
flutter/packages@353086c...6d02f03

2024-02-28 [email protected] Manual roll Flutter from c30f998 to d00bfe8 (32 revisions) (flutter/packages#6222)
2024-02-28 [email protected] [google_maps_flutter] Add `style` to widget (flutter/packages#6192)
2024-02-28 [email protected] Add library annotations for js interop (flutter/packages#6216)
2024-02-28 [email protected] [google_map_flutter] Add style to widget - platform impls (flutter/packages#6205)
2024-02-28 [email protected] [google_maps_flutter_web] Migrate to package:web (flutter/packages#5254)
2024-02-28 [email protected] [pigeon] Remove heap allocation in generated C++ code (flutter/packages#6196)
2024-02-27 [email protected] [pigeon] Allows kotlin generator to skip error class generation (flutter/packages#6183)
2024-02-27 [email protected] [camerax] Implements `setExposureMode` (flutter/packages#6110)
2024-02-27 [email protected] Roll Flutter from b77560e to c30f998 (12 revisions) (flutter/packages#6211)
2024-02-26 [email protected] Add `InkResponse`, `Material` and fix `Opacity` (flutter/packages#6199)
2024-02-26 [email protected] [url_launcher] Add explicit imports of UIKit (flutter/packages#6208)
2024-02-26 [email protected] [pigeon] Fix tool hangs on verbose sub-processes (flutter/packages#6198)
2024-02-26 [email protected] [tool] Ignore GeneratedPluginRegistrant.swift for `format` (flutter/packages#6195)
2024-02-26 [email protected] Roll Flutter from 1e8dd1e to b77560e (8 revisions) (flutter/packages#6207)

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 [email protected],[email protected] 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
LouiseHsu pushed a commit to LouiseHsu/packages that referenced this pull request Mar 7, 2024
This PR migrates `google_maps_flutter_web` from `dart:html` to `package:web`.

The `google_maps` package does provide a beta version with support for `package:web`, which is currently what this PR is depending on. Before landing this change, we should change the dependency to the stable release version of that package.

Fixes flutter/flutter#137374

*If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*
N/A
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
This PR migrates `google_maps_flutter_web` from `dart:html` to `package:web`.

The `google_maps` package does provide a beta version with support for `package:web`, which is currently what this PR is depending on. Before landing this change, we should change the dependency to the stable release version of that package.

Fixes flutter/flutter#137374

*If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*
N/A
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App p: google_maps_flutter platform-web
Projects
None yet
Development

Successfully merging this pull request may close these issues.

google_maps_flutter_web: migrate to pkg:web from dart:html
4 participants