Fix error where querySourceFeatures has parameters omitted. #3540 #3542
Conversation
|
Can you please fix the linting errors? |
andrewharvey
force-pushed
the
querysourcefeatures-optional-parameters
branch
from
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.
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.
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.
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.
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.
andrewharvey
force-pushed
the
querysourcefeatures-optional-parameters
branch
from
249dae8
to
5e22957
Compare
…dant params entry
andrewharvey
force-pushed
the
querysourcefeatures-optional-parameters
branch
from
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
parametersoptional 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
paramsas otherwise it would show up in the generated docs separately.