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

queryRenderedFeatures: default params to empty object instead of throwing error #2979

Merged
merged 4 commits into from
Aug 11, 2016

Conversation

mapsam
Copy link
Contributor

@mapsam mapsam commented Aug 10, 2016

What changed

Added a default empty object {} to map.queryRenderedFeatures() if nothing is passed through. Currently gl-js throws an error if the query is missing a second parameter. In order to pass linting tests, I had to rename params to parameters within the logic of queryRenderedFeatures.

fixes #2969

Tests

Added two new tests that confirm the object is generated if it isn't passed through, and a test that confirms all parameters are preserved in the output (happy to remove this if it's unnecessary).

Refs

#2647

cc @lucaswoj @bhousel @mollymerp

@mapsam mapsam changed the title default options to empty object instead of throwing error queryRenderedFeatures: default params to empty object instead of throwing error Aug 10, 2016

map.queryRenderedFeatures(map.project(new LngLat(0, 0)), {foo: 'bar'});
});

t.end();
Copy link
Contributor

Choose a reason for hiding this comment

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

The shared map across these test cases make me a little nervous but I don't see any immediate harm coming of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lucaswoj I grabbed this from the two tests above without thinking much about it. Happy to find an alternative!

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally understood! I appreciate you sticking to convention. 😄 Let's leave these tests-as is for now.

@@ -644,7 +644,7 @@ Style.prototype = util.inherit(Evented, {
}

var includedSources = {};
if (params.layers) {
if (params && params.layers) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lucaswoj In the test here I hit the error Error: TypeError: Cannot read property 'layers' of undefined, which led to a check to ensure params exists in the first place.

@mapsam
Copy link
Contributor Author

mapsam commented Aug 11, 2016

Tests added @lucaswoj! I'm not 💯 confident on the implementation of this - would love your 👀 and thoughts about how to implement the defaults in a better way. I think I'm getting hung up on this line mostly.

@lucaswoj lucaswoj merged commit c301bb5 into master Aug 11, 2016
@lucaswoj lucaswoj deleted the queryRenderedFeatures-bug branch August 11, 2016 19:58
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.

Fix bug preventing "queryRenderedFeatures" from being called with no arguments
2 participants