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

[google_maps_flutter_web] Migrate to null-safety. #3726

Merged
ditman merged 30 commits into from
Mar 31, 2021
Merged

[google_maps_flutter_web] Migrate to null-safety. #3726

ditman merged 30 commits into from
Mar 31, 2021

Conversation

@ditman
Copy link
Member

@ditman ditman commented Mar 18, 2021 •
edited
Loading

This PR migrates the google_maps_flutter_web package to null safety. To do so, certain things needed to change:

  • The underlying google_maps plugin has been updated to ^5.0.0 ^5.1.0. Thanks @a14n!
  • Tests had to be partially re-written due to the differences in null-safety mockito, and how google_maps is now put together (mockito can't mock extension methods).

The following changes were applied to the mocking strategy of this package:

  • Use Mockito 5 to generate mocks of the classes defined in this package (mainly, geometry controllers)
  • Use partially initialized JS Objects from the js-interop layer (functioning "Fakes"). These are supported by the underlying Maps JS SDK, so we leverage that!

Issues

Fixes flutter/flutter#74261
Fixes flutter/flutter#75236
Fixes flutter/flutter#75247

Testing

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 Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin 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.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the 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.

@google-cla google-cla bot added the cla: yes label Mar 18, 2021
@ditman ditman force-pushed the google-maps-nnbd branch from 879f9a4 to a45705a Compare March 18, 2021 00:46
@ditman ditman mentioned this pull request Mar 18, 2021
Closed
3 tasks
a14n
a14n reviewed Mar 18, 2021
View reviewed changes
stuartmorgan-g
stuartmorgan-g suggested changes Mar 18, 2021
View reviewed changes
@ditman ditman requested a review from stuartmorgan-g March 19, 2021 18:01
@ditman
Copy link
Member Author

ditman commented Mar 19, 2021

I need to rebase this with the latest master, to accomodate the changes made in [ci] and web plugins.

@ditman ditman force-pushed the google-maps-nnbd branch from a635ba8 to 201e3e5 Compare March 20, 2021 00:02
@ditman
Copy link
Member Author

ditman commented Mar 20, 2021

(This should now attempt to run the modified tests in the build-web+drive-examples CHANNEL:master step!)

stuartmorgan-g
stuartmorgan-g suggested changes Mar 24, 2021
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Mostly small comments. I'm still concerned at a high level by the amount of ! usage in ways that are not locally obviously correct (e.g., not cases like using a field right after checking that it's not null).

A lot of that is driven by the dispose system. I think it would be worth trying to separate the objects that have that pattern into a wrapper and an impl, where the impl has essentially all of the logic and all the fields are non-null, and the wrapper forwards to that object, and has dispose throw the whole impl away. It might be relatively straightforward to change?

This was referenced Mar 29, 2021
Closed
Merged
@ditman ditman force-pushed the google-maps-nnbd branch from f0ac203 to a69a8d0 Compare March 30, 2021 22:28
@ditman
Copy link
Member Author

ditman commented Mar 30, 2021

A lot of that is driven by the dispose system. I think it would be worth trying to separate the objects that have that pattern into a wrapper and an impl, where the impl has essentially all of the logic and all the fields are non-null, and the wrapper forwards to that object, and has dispose throw the whole impl away. It might be relatively straightforward to change?

@stuartmorgan for now, I ended up guarding all the ! with asserts. The plugin seems a little bit more throwy now than before, but all tests continued normally without real changes (in fact, I added tests to trigger some of those asserts). I'd rather not add another level of indirection to the controllers; what you're describing is essentially what the code is already doing (with the internal gmaps instances).

This could benefit from a refactor more in the style that you did with the geometry stuff in the core package (all "geometry" shares a set of base classes that have some default behavior, like dispose), but I'd rather introduce that change in another PR. This one is complex enough as is :/

@ditman ditman requested a review from stuartmorgan-g March 30, 2021 22:34
stuartmorgan-g
stuartmorgan-g approved these changes Mar 31, 2021
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

Okay, LGTM then. I do think followup refactoring to reduce the use of ! would be helpful, but I'm fine with proceeding as-is.

ditman added 19 commits March 31, 2021 11:33
@ditman
Add mocks, and the regen script
b0ff2f6
@ditman
Migrate google_maps_controller_test.dart
d9885b2
@ditman
Call regen_mocks from run_test. Document Mocks.
77e9702
@ditman
Add license to regen_mocks.sh
86f79ee
@ditman
Remove tests that don't apply in a post-null-safety world.
2bd4073
@ditman
Cleanup lies from pubspec comments.
a933c52
@ditman
No need for left-hand-side type.
dce7369
@ditman
Make return type slightly more strict in _mapStyles convert function.
f0abbf7
@ditman
Prevent calling methods on Geometry controllers after calling remove.
b17d888
@ditman
Some more PR reviews. dartfmt -w .
c33fc21
@ditman
Remove unused _nullZoom
5a08e9e
@ditman
Update to google_maps 5.1.0
fc41b15
@ditman
Add licenses.
a6c2a7e
@ditman
Single hash for license header in bash scripts.
151c74f
@ditman
Make build work in stable, and build+test in master.
a991500
@ditman
Address PR comments (needs tests)
65b6df0
@ditman
Test latest assertions after disposal of maps controller.
870e759
@ditman
dartfmt -w .
52f6dbd
@ditman
Revert no-sound-null-safety in build_all_plugins_app
0f701b1
@ditman
Copy link
Member Author

ditman commented Mar 31, 2021

Rebasing with the latest master to resolve example/pubspec.yaml conflict.

@ditman ditman force-pushed the google-maps-nnbd branch from 479a543 to 0f701b1 Compare March 31, 2021 18:35
@ditman ditman merged commit dc22884 into flutter:master Mar 31, 2021
@ditman ditman deleted the google-maps-nnbd branch March 31, 2021 20:50
@ditman
Copy link
Member Author

ditman commented Mar 31, 2021

Published as google_maps_flutter_web: ^0.3.0

https://pub.dev/packages/google_maps_flutter_web/versions/0.3.0

@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 31, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 31, 2021
@engine-flutter-autoroll
dc22884 [google_maps_flutter_web] Migrate to null-safety. (flutter/pl…
63ac498 
…ugins#3726)
fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Apr 1, 2021
@engine-flutter-autoroll
dc22884 [google_maps_flutter_web] Migrate to null-safety. (flutter/pl…
5740a62 
…ugins#3726) (#79488)
fotiDim pushed a commit to fotiDim/plugins that referenced this pull request Sep 13, 2021
@ditman @fotiDim
[google_maps_flutter_web] Migrate to null-safety. (flutter#3726)
964b7d6 
* Updates JS-interop package to google_maps ^5.1.0
* Breaking changes:
  * The property `icon` of a `Marker` cannot be `null`. Defaults to `BitmapDescriptor.defaultMarker`
  * The property `initialCameraPosition` of a `GoogleMapController` can't be `null`. It is also marked as `required`.
  * The parameter `creationId` of the `buildView` method cannot be `null` (this should be handled internally for users of the plugin)
  * Most of the Controller methods can't be called after `remove`/`dispose`. Calling these methods now will throw an Assertion error. Before it'd be a no-op, or a null-pointer exception.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@cyanglaz cyanglaz Awaiting requested review from cyanglaz

2 more reviewers

@a14n a14n a14n left review comments

@stuartmorgan-g stuartmorgan-g stuartmorgan-g approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes p: google_maps_flutter platform-web

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

☂️ Migrate all first-party plugins to NNBD [google_maps_flutter] Migrate to NNBD [google_maps_flutter_web] Migrate to google_maps-5.x

3 participants

@ditman @stuartmorgan-g @a14n