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

Fixes to JS-interop code. #6

Conversation

ditman
Copy link

@ditman ditman commented Apr 27, 2024

Basically the changes were:

  • Do not use the incompatible types in external definitions.
  • Ensure that the methods that expose Dart types do the translation .toJS as required.

Some other minor tweaks.

(Verified by running the demo app in my desktop, but please, do test on your end as well!)

((/cc @srujzs))

Copy link

@srujzs srujzs left a comment

Choose a reason for hiding this comment

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

It's unfortunate how many indirections you need to do because you can't put package:js types on the external boundary. :(


/// The [MarkerClustererOptions] object used to initialize [MarkerClusterer].
///
/// See: https://googlemaps.github.io/js-markerclusterer/interfaces/MarkerClustererOptions.html
@JS()
Copy link

Choose a reason for hiding this comment

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

nit: The @JS is optional unless you're doing renaming, but up to you if you want to keep it for signalling that it's an interop type. The @anonymous doesn't do anything, however, and will be a warning once dart-lang/sdk#55581 is resolved.

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

// TODO(srujzs): Needed for https://github.com/dart-lang/sdk/issues/54801. Once
Copy link

Choose a reason for hiding this comment

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

FYI - if you update the SDK constraints to 3.3.1, this isn't needed.

Copy link

@jokerttu jokerttu Apr 29, 2024

Choose a reason for hiding this comment

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

Seems like any of the packages do not yet use constraints bigger than ^3.3.0.
I'm not sure if the update needs to be applied to all plugins / ci or if updating just this one package would be ok.
I'll ask about this on the main PR.

Copy link
Author

Choose a reason for hiding this comment

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

We can upgrade for this package only, since dependencies are not pinned, and 3.3.1 is picked up by ^3.3.1 anyways!

@jokerttu
Copy link

This looks great... merging and finalizing the update to js_interlop in the target branch.

@jokerttu jokerttu merged commit ef29656 into CodemateLtd:feature/google_maps_flutter_web_marker_clustering_js_interop Apr 29, 2024
3 checks passed
@ditman ditman deleted the feature/google_maps_flutter_web_marker_clustering_js_interop branch April 29, 2024 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants