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

Allow package web: ^1.0.0 #135

Merged
merged 4 commits into from
Jul 24, 2024
Merged

Allow package web: ^1.0.0 #135

merged 4 commits into from
Jul 24, 2024

Conversation

ditman
Copy link
Contributor

@ditman ditman commented Jul 23, 2024

Hey @a14n!

I'm looking at updating flutter/packages to the latest major version of package:web, and I think I need a small change in google_maps: allowing web 1.0.0.

It seems the only other changes needed are to the examples, where the use of innerHTML has gone from String to JSAny (so Trusted Types are supported), but it seems the rest is all right.

I re-ran the build script(s) but nothing else seems to have changed:

dit@dit:/work/a14n/google_maps$ dart --no-sound-null-safety tool/generate_lib.dart 
Setting VM flags failed: Unrecognized flags: sound_null_safety
dit@dit:/work/a14n/google_maps$ dart tool/generate_lib.dart 
dit@dit:/work/a14n/google_maps$ dart run build_runner build --delete-conflicting-outputs -v lib
Building package executable... (6.7s)
Built build_runner:build_runner.
[INFO] Entrypoint:Generating build script...
[INFO] Entrypoint:Generating build script completed, took 465ms

[INFO] Bootstrap:Precompiling build script......
[INFO] Bootstrap:Precompiling build script... completed, took 6.4s

[FINE] Bootstrap:Core package locations file does not exist
[INFO] BuildDefinition:Initializing inputs
[INFO] BuildDefinition:Building new asset graph...
[INFO] BuildDefinition:Building new asset graph completed, took 2.0s

[INFO] BuildDefinition:Checking for unexpected pre-existing outputs....
[INFO] BuildDefinition:Checking for unexpected pre-existing outputs. completed, took 0ms

[INFO] Build:Running build...
[INFO] Build:Running build completed, took 391ms

[INFO] Build:Caching finalized dependency graph...
[INFO] Build:Caching finalized dependency graph completed, took 304ms

[INFO] Build:Succeeded after 705ms with 0 outputs (2522 actions)

I've also updated the README-dev so it doesn't fail with the VM flags error, but that may be because I'm in a Dart SDK from the future (whatever is bundled with flutter master).

(As usual, thanks for the package!!)

@ditman
Copy link
Contributor Author

ditman commented Jul 23, 2024

@ditman
Copy link
Contributor Author

ditman commented Jul 24, 2024

Also a small question: why are there isFooDefined() methods, instead of making .foo nullable? (For example: "isTiltDefined()" in the map object?)

Is there a difference between undefined and null in the JS SDK?

@a14n a14n merged commit cd576db into a14n:new-js-interop Jul 24, 2024
@a14n
Copy link
Owner

a14n commented Jul 24, 2024

Thanks!

Also a small question: why are there isFooDefined() methods, instead of making .foo nullable? (For example: "isTiltDefined()" in the map object?)

Usually those properties have always values once the map is initialized (with a center and zoom level) so 99% of the time it is non-null. Having isFooDefined() allows checking nullity in the rare cases where you access the property before the map is initialized. Once initialized you now avoid a lot of null checks. For instance, in examples, those isXxxDefined() are almost never used.

@a14n
Copy link
Owner

a14n commented Jul 24, 2024

8.0.0-dev.3 published

@ditman ditman deleted the allow-web-1 branch July 24, 2024 19:33
@ditman
Copy link
Contributor Author

ditman commented Jul 24, 2024

Having isFooDefined() allows checking nullity in the rare cases where you access the property before the map is initialized.

This sounds good to me, my only concern is that these methods seem a little bit hard to find in the API. In the flutter plugin most of the usages are in tests, or on my wacky "convert.dart" file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants