Skip to content

Correct feature state id type to number - #7106

Merged
asheemmamoowala merged 6 commits into
mapbox:masterfrom
bfrengley:feature-state-id-type
Aug 16, 2018
Merged

Correct feature state id type to number#7106
asheemmamoowala merged 6 commits into
mapbox:masterfrom
bfrengley:feature-state-id-type

Conversation

@bfrengley

Copy link
Copy Markdown
Contributor

Currently, string feature ids are not propagated through the transformation from GeoJSON to vector tiles (#2716, #6960) and are not supported by vector tiles at all. However, setFeatureState / getFeatureState indicate that they require string ids.

This pull request updates setFeatureState / getFeatureState to require unsigned numeric ids.

  • briefly describe the changes in this PR
  • document any changes to public APIs

@mourner mourner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was wondering the same thing! @asheemmamoowala any objections?

@asheemmamoowala asheemmamoowala left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bfrengley Thank you for submitting this PR! Just one small change to the allowed input types for Map#setFeatureState.

Comment thread src/style/style.js
if (feature.id == null || feature.id === "") {
this.fire(new ErrorEvent(new Error(`The feature id parameter must be provided.`)));
if (feature.id == null || feature.id < 0) {
this.fire(new ErrorEvent(new Error(`The feature id parameter must be provided and non-negative.`)));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think Map#setFeatureState should still accept string types that can be coerced to integers. This would allow what is otherwise a valid id for a GeoJSON feature to be input to the API without explicit conversion.

Based on #4581:

// If the feature has a top-level `id` property, copy it over, but only
// if it can be coerced to an integer, because this wrapper is used for
// serializing geojson feature data into vector tile PBF data, and the
// vector tile spec only supports integer values for feature ids --
// allowing non-integer values here results in a non-compliant PBF
// that causes an exception when it is parsed with vector-tile-js
if ('id' in feature && !isNaN(feature.id)) {
this.id = parseInt(feature.id, 10);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

While I am making this change: is there a reason Map#getFeatureState does not do the same validation of feature.id that is done in Map#setFeatureState? I see #6959 only addresses requiring an id in Map#setFeatureState

@asheemmamoowala asheemmamoowala Aug 14, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@bfrengley that seems like a fix worth putting in as well. Thank you!

@mourner

mourner commented Aug 15, 2018

Copy link
Copy Markdown
Member

Rebasing the commits of this branch on top of the base branch cannot be performed automatically due to conflicts encountered while reapplying the individual commits from the head branch.

@bfrengley can you please rebase on top of master for clean merging?

@bfrengley

Copy link
Copy Markdown
Contributor Author

@mourner done.

@asheemmamoowala
asheemmamoowala merged commit 103a2e7 into mapbox:master Aug 16, 2018
@bfrengley
bfrengley deleted the feature-state-id-type branch August 16, 2018 22:59
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