-
-
Notifications
You must be signed in to change notification settings - Fork 663
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
migrates geolocator_web to package:web #1475
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @josh-burton,
Sorry for the delay and thank you for creating this PR. In general the changes look good, I have made a few small remarks which would be great if you can address those.
} on html.PositionError catch (e) { | ||
throw convertPositionError(e); | ||
} catch (e) { | ||
completer.completeError(e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please convert the error to one of the custom exceptions defined in the convertPositionError
method before completing the completer.
This will throw a known exception to the application developers so they don't have to update their application logic and handle unexpected exceptions solely for the geolocator_web plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've returned a platform exception so the caught exception can be passed to the detail field. Another option could be the PositionUpdateException
exception
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the PositionUpdateException
would be an excellent fit here. Using the PlatformException
causes developers to worry about the LOCATION_UPDATE_FAILURE
error code while their code base should already be handling the PositionUpdateException
(if written correctly of course).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, my only concern there is the underlying exception will be lost. Perhaps we could add an optional cause field to the PositionUpdateException?
Thanks for the review. I believe I've addressed them now. |
When merged solves #1513 |
@mvanbeusekom I see the workflows are failing. I had bumped the geolocator_web version to 4.0 as its a breaking change, but as 4.0 is not published the workflows now error. What's the correct fix here? |
The best would be to split the changes into 2 PRs. One updating the geolocator_web package and a second bumping the version for the geolocator package.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR migrates from dart:html to package:web and js_interop.
A few notes/questions:
flutter test --platform chrome
, otherwise errors are thrown as js_interop can't be used in the dart vm.Pre-launch Checklist
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is does not need version changes.CHANGELOG.md
to add a description of the change.///
).main
.dart format .
and committed any changes.flutter analyze
and fixed any errors.