CARTO: Adapt fetchMap for supporting HeatmapTileLayer

[jmgaya]

Background

• After adding support for the HeatmapTileLayer, we should update the fetchMap functionality too

Change List

• Adapt fetchMap to support HeatmapTileLayer

[coveralls]

coverage: 89.56% (-0.003%) from 89.563%
when pulling 55daf5d on jmgaya/adapt-fetchmap-heatmap-tile
into f1a163a on master.

[felixpalmer]

Nice job :) just a few small comments

[coveralls]

coverage: 89.564% (+0.001%) from 89.563%
when pulling 6c250df on jmgaya/adapt-fetchmap-heatmap-tile
into f1a163a on master.

[coveralls]

coverage: 89.568% (+0.005%) from 89.563%
when pulling cff8c6e on jmgaya/adapt-fetchmap-heatmap-tile
into f1a163a on master.

[felixpalmer]

I don't think you need the constants, instead do:

type LayerType = 'raster' | 'quadbin' | 'h3' | 'heatmapTile';

and then type LAYER_FROM_LAYER_TYPE as Record<LayerType, Layer>

[jmgaya]

I don't think you need the constants

But that means repeating strings like 'raster' everywhere, so isn't it better to create a constant and use it 🤔 ?

I see much more valuable holding this into constants, because human mistakes are lower, and moreover, you don't have to remember what if it was heatmapTile, heatmap-tile or heatmap_tile, it just works 🤷

[jmgaya]

then type LAYER_FROM_LAYER_TYPE as Record<LayerType, Layer>

Isn't that inferred automagically by Typescript?

[felixpalmer]

Yes, that is the pattern we've adopted since the move to Typescript, e.g. take a look at the defaultProps in Layer:

deck.gl/modules/core/src/lib/layer.ts

Lines 145 to 158 in 963bbcc

	operation: 'draw',
	onHover: {type: 'function', value: null, optional: true},
	onClick: {type: 'function', value: null, optional: true},
	onDragStart: {type: 'function', value: null, optional: true},
	onDrag: {type: 'function', value: null, optional: true},
	onDragEnd: {type: 'function', value: null, optional: true},
	coordinateSystem: COORDINATE_SYSTEM.DEFAULT,
	coordinateOrigin: {type: 'array', value: [0, 0, 0], compare: true},
	modelMatrix: {type: 'array', value: null, compare: true, optional: true},
	wrapLongitude: false,
	positionFormat: 'XYZ',
	colorFormat: 'RGBA',

Here we directly type the string, and rely on TypeScript to enforce that it is correct. If you try changing one of those types, the compiler will stop you.

With my approach, you reduce the 9 lines defining (needless) constants into one and you also verify that LAYER_FROM_LAYER_TYPE contains all the required mappings

[jmgaya]

Understood! Added the Record<...>

I have to disagree with the "needless" of defining constants. Defining constants has many benefits:

• It's self documenting, because constants could have descriptive names, rather than "just values", for example, the const UNKNOWN_COLOR = '#868d91'; in this same file
• It's a single point of change, in case the LayerType gets modified, meaning with your proposal we'll have to modify several places
• We are avoiding human mistakes as typos, because maybe in some places it's typed as string, but not as LayerType
• Maybe I'm wrong, but modern JS engines optimize better constants than hard-coded literals
• Refactors are much easier with constants, because of the single point of change

In this particular case, all the layer types were repeated at least three times, that's exactly why I thought a const could be a good idea 🤷

Said so, if that's the agreement, I'll follow it 👍

[felixpalmer]

In the case of "magic" values you may have a points, but

const layerType: LayerType = 'raster';

is more readable as I actually know what the variable will hold. With a constant I have to go look it up.

As for the single point of change, we just update the type and fix the errors the compiler instantly throws up.

Humans shouldn't type it as string 😉 it won't pass code review (as you can see!)

[coveralls]

coverage: 89.567% (+0.004%) from 89.563%
when pulling 5a6777b on jmgaya/adapt-fetchmap-heatmap-tile
into f1a163a on master.

[coveralls]

coverage: 89.565% (+0.002%) from 89.563%
when pulling 9cdbe7f on jmgaya/adapt-fetchmap-heatmap-tile
into f1a163a on master.

[coveralls]

coverage: 89.564% (+0.001%) from 89.563%
when pulling 6b32032 on jmgaya/adapt-fetchmap-heatmap-tile
into f1a163a on master.

[coveralls]

coverage: 89.564% (+0.001%) from 89.563%
when pulling ddb0917 on jmgaya/adapt-fetchmap-heatmap-tile
into f1a163a on master.

[coveralls]

coverage: 89.564% (+0.001%) from 89.563%
when pulling 7b19588 on jmgaya/adapt-fetchmap-heatmap-tile
into f1a163a on master.

[coveralls]

coverage: 89.564%. remained the same
when pulling e121a3a on jmgaya/adapt-fetchmap-heatmap-tile
into a745330 on master.