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

Geocoding error handling #345

Merged
merged 3 commits into from
Aug 21, 2023

Conversation

sronveaux
Copy link
Collaborator

Here's some kind of follow-up to #343 where a flaky behaviour was observed during Geocoder unit tests.
Sometimes a strange error was thrown : TypeError: this.results.forEach is not a function

This happened from the query function inside the GeocoderController called from the Geocoder.vue component where the actual request was made like this:

try {
  const response = await json(ajax);
  return this.provider.handleResponse(response)
} catch (err) {
  return err
}

But with this code, if the underlying fetch was in error, the exception was caught then returned which made the Promise from the Geocoder component resolve instead of reject. So the error was considered as a result but was an Error instance instead of the expected Array of results...

This was simply corrected by removing the try - catch. If an error is thrown, it is practically caught inside the querySelections method of the Geocoder component as planned first.

The other unit tests for the geocoder have been slightly refactored to use a fake request instead of doing multiple API calls to Nominatim.
Some could argue that there is still a test making a real request and that this shouldn't be done inside unit tests but as this is the one which unveiled the bug corrected in #343 I thought making a real request was valuable in this case.

Please note that this final implementation is perhaps not the best one (onQueryError() does nothing except making a call to trace()) but the bug is corrected and the component should behave as it was designed at first...

Copy link
Collaborator

@chrismayer chrismayer left a comment

Choose a reason for hiding this comment

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

Thanks for this @sronveaux. I just tested your change and this works quite well.

@chrismayer chrismayer merged commit b53e8ce into wegue-oss:master Aug 21, 2023
1 check passed
@sronveaux sronveaux deleted the geolocationErrorHandling branch August 21, 2023 08:55
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