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

Imagery provider API consistency #4812

Closed
hpinkos opened this issue Jan 5, 2017 · 5 comments · Fixed by #6203
Closed

Imagery provider API consistency #4812

hpinkos opened this issue Jan 5, 2017 · 5 comments · Fixed by #6203

Comments

@hpinkos
Copy link
Contributor

hpinkos commented Jan 5, 2017

From #2814, @mramato:

It is kind of weird and inconsistent that we use functions for some types classes for others. I know perfect is the enemy of the good and all that, but it makes our API harder to use.

I think we should change createOpenStreetMapImageryProvider and createTileMapServiceImageryProvider back into classes so they match the other imagery provider types. They could inherit from UrlTemplateImageryProvider so we don't have to worry about introducing more duplicate code.

@hpinkos hpinkos added the cleanup label Jan 5, 2017
@hpinkos
Copy link
Contributor Author

hpinkos commented Jan 5, 2017

From #2814, @kring:

Point 2 could be addressed with @hpinkos's suggestion: inherit from UrlTemplateImageryProvider rather than aggregating it. AFAIK this would be the first use of inheritance anywhere in Cesium, though.

Is there any concern with introducing inheritance?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jan 5, 2017

Is there any concern with introducing inheritance?

Worth a try since this is a simple case; I don't think inheritance will make it out of control.

The doc will need to be duplicated though, right?

@mramato
Copy link
Contributor

mramato commented Jan 5, 2017

The doc will need to be duplicated though, right?

JSDoc supports doc inheritance via the inherritdoc tag. http://usejsdoc.org/tags-inheritdoc.html

When we started Cesium we were concerned about inheritance and performance; but I'm not sure those concerns are valid any longer. We should consider getting more aggressive about inheritance in the future and making out interfaces actual base instances.

@thw0rted
Copy link
Contributor

Did this ever go anywhere? Also, createOpenStreetMapImageryProvider is maybe the only place that takes a URL as a string, not a Resource, and is thus completely impossible to load through a proxy.

@hpinkos
Copy link
Contributor Author

hpinkos commented Aug 13, 2019

@thw0rted I opened a PR a while ago but it hasn't been a priority for us to review and merge it #6203

If createOpenStreetMapImageryProvider doesn't take a Resource, that's a quick fix we should be able to do in the meantime. @ProjectBarks would you have a minute to look into that? All other imagery providers should take a Resource

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants