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

Support custom layers in setStyle #7576

Open
gampleman opened this issue Nov 9, 2018 · 4 comments
Open

Support custom layers in setStyle #7576

gampleman opened this issue Nov 9, 2018 · 4 comments

Comments

@gampleman
Copy link
Contributor

Motivation

Custom layers are great, but it seems like one can only be added with addLayer. However many vdom applications use setStyle to manage changes with layers. This seems to be overwriting the custom layer which then disappears.

Design Alternatives

Ideally, setStyle could be used with custom layers. However, there is a question of how diffing should work. I think there are two alternatives:

  1. match up the layer by id and then do an equality check - if it doesn't match remove the old and add the new.
  2. Add diff(otherLayer) -> patch and patch(patch) optional methods on custom layers. This would allow for possibly more performant mutation, but would make for much more API surface.

Design

I think going with option 1 would be a lot simpler in the short run, and waiting to see demand for something more complicated would help.

Mock-Up

Concepts

Implementation

@asheemmamoowala
Copy link
Contributor

asheemmamoowala commented Nov 9, 2018

Duplicate of #4006.

That ticket doesn't specifically call out layers of CustomLayerInterface type, but I believe it should include them in any solution.

cc @ansis

@gampleman
Copy link
Contributor Author

I don't believe this is a duplicate unless I misunderstand that issue.

I think what I'm asking is:

Currently to add a custom layer one does:

map.addLayer(myCustomLayer);

I'm asking to also support:

map.setStyle({
  version: 8,
  sources: {},
  layers: [myCustomLayer] 
});

(This is a toy example, normally that would include a whole bunch of other sources and layers.

@asheemmamoowala
Copy link
Contributor

Does myCustomLayer refer an object that implements the CustomLayerInterface type? If so, you are right, it is not a duplicate.

The custom layer type is a mapbox-gl-js specific runtime type, and not a part of the Style specification. The style-spec is used for rendering maps across multiple platforms and does not include any types that are platform dependent.

Adding support for that type in a setStyle() invocation with inline values would be possible, but it would not create confusion when invoked with a URL. It is for the same reason that canvas source types are not supported in setStyle, and have to be added using runtime APIs.

@gampleman
Copy link
Contributor Author

Does myCustomLayer refer an object that implements the CustomLayerInterface type?

Yes.


I see your point.

However, when using mapboxgl using reactive style programming* (usually through a wrapper), using the addLayer api is not straightforward. One has to implement their own diffing (which I think the Uber wrapper used to do at some point) to support that. That isn't ideal, since it duplicates code that is already in mapboxgl and adds a lot more complexity to the wrapper (and a lot more opportunity to break stuff).


* By reactive I mean where the programmer writes a function from the application state to a style spec object and the runtime takes care of updating the map accordingly.

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

No branches or pull requests

4 participants