-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Fix error where querySourceFeatures has parameters omitted. #3540 #3542
Fix error where querySourceFeatures has parameters omitted. #3540 #3542
Conversation
Can you please fix the linting errors? |
a1d6bb9
to
249dae8
Compare
Sorry I ran the unit tests but failed to run the full test suite. Should be fixed now, I just overwrote the commits so not sure if ci automatically reruns or not... |
@@ -575,7 +575,7 @@ class Map extends Camera { | |||
* representing features within the specified vector tile or GeoJSON source that satisfy the query parameters. | |||
* | |||
* @param {string} sourceID The ID of the vector tile or GeoJSON source to query. | |||
* @param {Object} parameters | |||
* @param {Object} [parameters] |
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.
Got it. This is more of a feature addition than a bug fix. We never supported or claimed to support omitting parameters
. 👍
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 suppose so it is 😉
Given parameters.sourceLayer
and parameters.filter
were optional I incorrectly assumed I could omit parameters
entirely but I guess the JS doc says you still need to provide an empty object.
@@ -646,6 +647,50 @@ test('Map', (t) => { | |||
t.end(); | |||
}); | |||
|
|||
t.test('#querySourceFeatures', (t) => { |
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.
querySourceFeatures
tests are currently split between tile.test.js
and style.test.js
. Could you move this test to one of those two places?
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 see. I opted to put the test in map since I thought it was best to test the top level public interface, rather than a private interface used internally where it's not clear what arguments are optional.
That said, I've moved the test to tile.test.js
and made it much smaller and simpler since I don't need a whole bunch of setup/data in the unit test to avoid it taking performance shortcuts which skip what I needed to test.
I've just overwritten these commits rather than making new commits to avoid a rebase later.
249dae8
to
5e22957
Compare
…dant params entry
5e22957
to
c87a477
Compare
Looks great! Thank you so much! |
Launch Checklist
See #3540 for background on what this fixes.
There are a couple of places where this could be fixed, so I'm not sure where is best higher or lower in the call stack. I've opted to place it at the bottom of the stack.
The test is quite verbose because if you don't add any features to the source and if this source isn't included in a source the code path which I ran into this error isn't triggered.
If you remove the fix the test fails if you include this fix the test passes.
I've included a second commit which makes
parameters
optional although I'm not sure if this is correct as it's only optional for GeoJSON sources as per current documentation.I needed to rename
params
as otherwise it would show up in the generated docs separately.