CartoDB / toolkit

JS library to interact with CARTO APIs in a simple way
https://toolkit-wheat.now.sh
BSD 3-Clause "New" or "Revised" License
9 stars 3 forks source link

Move logic of tile instantiation to sources #79

Closed alasarr closed 4 years ago

alasarr commented 4 years ago

The idea behind this PR is to move out from Layer.ts the logic related with data sources.

Why? Because if we want to include another kind of layer sources (as Data Observatory layer or a geojson) we can the layer object to have the same interface for styling, to link with deck, with popup, etc..

So with this idea, we've all the logic related to sources at the sources object instead of the Layer Pending stuff:

alasarr commented 4 years ago

BTW, This is my first code in TS, feel free to suggest any typo, improvement...

alasarr commented 4 years ago

Ready for reviwe

jesusbotella commented 4 years ago

I have not reviewed all new changes because I think that there's something to important to discuss here.

As per the latest changes, we are embedding a new type of layer (DataObservatoryLayer) which inherits from a MVTLayer inside main our Layer class. I think that the mechanism of checking what kind of source is the one that we have and instantiate a layer based on that is not optimal because we can have many if clauses that we don't want to.

So, given that we are refactoring our sources and layers, why don't we allow each source to define the type of layer that it wants to use and have an MVT Layer by default?

I don't like this type of source checks (and I think we should avoid them at all times):

if (this._source instanceof CARTOSource) {
  this._deckLayer = new MVTLayer(layerProperties);
} else if (this._source instanceof DataObservatorySource) {
  this._deckLayer = new DataObservatoryLayer(layerProperties);
} else {
  throw Error('Unsupported source instance');
}

For example, if we have a GeoJSON source (and we will eventually have it), we will have to add another if clause to the ones we already have. This can be fixed with a factory, but I think it is smarter if we allow each source to define the type of layer that wants to use.

@VictorVelarde @alasarr