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

Explicit source options overwrite #8232

Merged
merged 6 commits into from
Jun 14, 2019
Merged

Conversation

jingsam
Copy link
Contributor

@jingsam jingsam commented May 9, 2019

Explicit source options should taken precedence over TileJSON. See details in #8192

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/maps-design if this PR includes style spec changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port

"line": 12
},
{
"message": "sources.no-unknown-properties-with-url.foo: a source with a \"url\" property may not include a \"foo\" property",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is unknown properties support needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think foreign members (unknown properties) in source options should be leaved as it is. Foreign members does not affect source's functionality and add extensibility for metadata or custom sources. As we don't invalidate foreign members in geojson、image、video sources, invalidate foreign members in vector, raster, raster-dem seems inconsistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jingsam Sorry for the delay in getting back to this. Allowing foreign members in the TileJSON does not seem necessary as part of this PR. It is preferable to have strict validation to ensure that unnecesary or incorrect properties are not being added to such files.

@asheemmamoowala
Copy link
Contributor

cc @mapbox/studio

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

Good to go except the foreign properties test.

@jingsam
Copy link
Contributor Author

jingsam commented May 31, 2019

@asheemmamoowala I have invalid foreign members in source options when url is defined. As you can see, I have enum so many possible valid members. This code is fragile as someday we may need new property in test cases or application. If we now restrict properties allowed in sources options, that may make sources options can not be extended. Actually, in mapbox-style-spec, there is no limitation on foreign properties. So limit in here is sounds unreasonable. Forregin properties are hurtless, if mapbox-gl-js does not use them, take as it is but not restrict them.

if ('url' in value) {
for (const prop in value) {
if (['type', 'url', 'tiles', 'minzoom', 'maxzoom', 'attribution', 'mapbox_logo', 'bounds', 'scheme', 'tileSize', 'encoding'].indexOf(prop) < 0) {
errors.push(new ValidationError(`${key}.${prop}`, value[prop], `a source with a "url" property may not include a "${prop}" property`));
}
}
}

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

@jingsam Thank you for sticking to your guns on this. Just rechecked the TilJSON spec. It states that

Implementations MUST treat unknown keys as if they weren't present.

LGTM!

Copy link
Contributor

@asheemmamoowala asheemmamoowala left a comment

Choose a reason for hiding this comment

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

Actually - could you add a render test to ensure that the over-ride properties take effect. An example that uses constrains maxzoom or bounds would work well with a small image size.

@jingsam
Copy link
Contributor Author

jingsam commented Jun 14, 2019

@asheemmamoowala I add a test, please check out.

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.

2 participants