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

Remove feature.id checks for event deduplication #298

Merged
merged 6 commits into from
Oct 14, 2019
Merged

Conversation

scottsfarley93
Copy link

@scottsfarley93 scottsfarley93 commented Oct 1, 2019

Deduplicating events by feature id was introduced in #229. While this prevents duplicate result events (tristen/suggestions#13), it introduces (at least) two undesirable side effects.

  1. When using a localGeocoder, the result event will only be emitted once, manifesting in poor user experience by not zooming to the location of the second selection. (Bug reports: multiple local geocoder search results broken #281, Local geocoder doesn't emit result event #278).
  2. If features share the same ID (as reported in result event not emitted for a two different results which share the same id #293), subsequent selections will not emit the result event.

This PR aims to fix the above two issues.

My initial thought is to use the place_name instead of the feature's ID as a field to deduplicate on. This should work for case (2) above, because the two addresses will have different place_names. However, for the local geocoder, if some features share the same place_name, the same behavior will continue to occur. While I expect that most users will have unique place names in their data, there is no requirement for uniqueness, so perhaps we should not rely on this alone.

Alternate approaches include:
(1) adding additional checks to see if the results came from the local Geocoder
(2) attempting (again) to solve the duplicate result emission in the suggestions library.

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • run npm run docs and commit changes to API.md
  • update CHANGELOG.md with changes under master heading before merging

@scottsfarley93
Copy link
Author

As per #293 (comment), it might make sense to use a stringified version of the whole feature as our de-duplication criterion. This would mean that users would not need to ensure uniqueness across their place_names, because the coordinates and other feature data would make each feature unique.

Tests to come.

@andrewharvey
Copy link
Collaborator

I think this is a reasonable approach.

@sashako
Copy link

sashako commented Oct 4, 2019

Hello, hope it's okay that I butt in. I've encountered this bug in a bit of a different context: If the user decided to legitimately enter the same address twice the second search flow will not trigger 'result' event. Have you considered clearing out this.lastSelected on user input or perhaps with just a long enough timeout - so you're still deduping consecutive events that come in rapid succession, but enable the user to enter the same address twice if they choose to do so.

@scottsfarley93
Copy link
Author

Hey @sashako thanks for jumping in! Could you describe a bit more about this use case? In what cases is the user attempting to enter the same address twice in a row? I've not yet considered this.

I'm not opposed to resetting the lastSelected property on input change. I think that will still avoid the duplicate result events emitted by suggestions.

@scottsfarley93
Copy link
Author

@sashako I added a change in 82be5eb to reset the lastSelected object on each keyDown event. Would you mind giving this a test and seeing if it fixes the issue you describe here, where you're unable to select the same feature twice in a row?

@andrewharvey
Copy link
Collaborator

@sashako I added a change in 82be5eb to reset the lastSelected object on each keyDown event. Would you mind giving this a test and seeing if it fixes the issue you describe here, where you're unable to select the same feature twice in a row?

I can't speak for @sashako, but I tested a use case where there is no id supplied, but the user can select the same result twice in a row, and this PR fixes the issue for me.

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.

3 participants