Carto: fetchMap to support custom basemaps #8856

zbigg · 2024-04-30T10:50:32Z

Support for custom and google basemaps in fetchMap.

Change List

Carto: fetchMap to support custom and google basemaps
Carto: tests/apps/carto-map: support for custom and google basemaps

felixpalmer · 2024-04-30T13:53:15Z

modules/carto/src/api/basemap.ts

+    'basemap-tile-source': {
+      type: 'raster',
+      tiles: [basemap.settings.url],
+      tileSize: 256


256 seems like a pretty small tile size, is that correct?

This is some magic number that we agreed upon and that works well for most sources we tested.

Anyway, i think we should make it confiruable.
I'll try to make it part of protocol, so it's already part of keplerMapConfig and we don't have to hardcode it here.

Good point.

Finally, i've decided that it's better to keep whole style, so app that generates keplerMapConfig has complete control over what style is in basemap (as side-effect, we don't need code that generates style in this module).

coveralls · 2024-05-06T13:44:22Z

coverage: 89.841% (+0.01%) from 89.829%
when pulling 1be5fc6 on zbigg/carto-fetchmap-custombasemaps
into d504327 on master.

zbigg · 2024-05-06T13:53:00Z

docs/api-reference/carto/fetch-map.md

@@ -90,9 +90,13 @@ When the map was last updated.

 The [view state](../../developer-guide/views.md#view-state).

-#### `mapStyle` (string) {#mapstyle}


Old mapStyle was actually an object with various internal properties (like styleType - an actual id of style).

New api is more robust and thus i propose to completly hide mapStyle in documentation.

(it's still returned for backwards compatibility, but it can be considered deprecated now)

I'm good hiding it! Shouldn't we also remove it or comment it from example in line 27?

I understand the API still returns it for compatibility but using it in the example without context looks confusing to me

modules/carto/src/api/fetch-map.ts

modules/carto/src/api/basemap.ts

modules/carto/src/basemap.ts

modules/carto/src/api/basemap.ts

modules/carto/src/basemap.ts

modules/carto/src/api/basemap.ts

felixpalmer · 2024-05-07T08:17:29Z

modules/carto/src/basemap.ts

+
+  if (visibleLayerGroups && someLayerGroupsDisabled(visibleLayerGroups)) {
+    try {
+      const originalStyle = await fetch(styleUrl, {


Rather than using fetch directly, use requestWithParameters if possible along with an APIErrorContext

requestWithParameters does lots of things, like passing ?v= (deck version), falling back to POST not allowing setting mode.

I guess that using just fetch is simpler than artificially extending requestWithParameters to become another fetch-like univeral API.

(added usage of errorContext though)

The other major thing requestWithParameters provides is caching of repeated requests, but in all the current examples this doesn't matter and we only call fetchMap once, so I think it's optional here. 👍🏻

Well, we do have browser cache already. If we really want to use current cache, i would factor-it-out as separate "fetchWithCache" (and only with cache), but without carto-specific features (?v=..., fallback to POST ...),

And when trying to see how requestWithParameters can be adapted/simplified, i've found that its cache is not-configurable and doesn't have any means to clear it - i think we should improve it.

Anyway i guess that in current state of affair it's better to rely on browser cache.

donmccurdy

Thanks for the additional unit tests!

modules/carto/src/api/parse-map.ts

donmccurdy · 2024-05-08T14:40:00Z

modules/carto/src/basemap.ts

+
+  if (visibleLayerGroups && someLayerGroupsDisabled(visibleLayerGroups)) {
+    try {
+      const originalStyle = await fetch(styleUrl, {


The other major thing requestWithParameters provides is caching of repeated requests, but in all the current examples this doesn't matter and we only call fetchMap once, so I think it's optional here. 👍🏻

modules/carto/src/api/basemap.ts

donmccurdy

Added a few optional suggestions, then LGTM, thanks!

docs/api-reference/carto/basemap.md

modules/carto/src/api/basemap.ts

modules/carto/src/api/types.ts

modules/carto/src/index.ts

zbigg force-pushed the zbigg/carto-fetchmap-custombasemaps branch from f03cd54 to e683c7b Compare April 30, 2024 11:01

zbigg requested review from donmccurdy, padawannn, felixpalmer and srtena April 30, 2024 12:39

zbigg force-pushed the zbigg/carto-fetchmap-custombasemaps branch from c794f07 to 0f04eb6 Compare April 30, 2024 13:11

felixpalmer reviewed Apr 30, 2024

View reviewed changes

zbigg force-pushed the zbigg/carto-fetchmap-custombasemaps branch from 4d038e5 to bb64b83 Compare April 30, 2024 15:34

zbigg requested a review from felixpalmer May 6, 2024 11:25

zbigg force-pushed the zbigg/carto-fetchmap-custombasemaps branch 2 times, most recently from 4972f03 to 7adbf11 Compare May 6, 2024 13:37

zbigg force-pushed the zbigg/carto-fetchmap-custombasemaps branch 2 times, most recently from b709e7a to 4960483 Compare May 6, 2024 13:49

zbigg commented May 6, 2024

View reviewed changes

zbigg force-pushed the zbigg/carto-fetchmap-custombasemaps branch from 4960483 to fc64562 Compare May 6, 2024 13:53

zbigg marked this pull request as ready for review May 6, 2024 13:54

zbigg commented May 6, 2024

View reviewed changes

modules/carto/src/api/fetch-map.ts Show resolved Hide resolved

zbigg force-pushed the zbigg/carto-fetchmap-custombasemaps branch from fc64562 to f8ec0c8 Compare May 6, 2024 13:58

donmccurdy reviewed May 6, 2024

View reviewed changes

modules/carto/src/api/basemap.ts Show resolved Hide resolved

modules/carto/src/basemap.ts Outdated Show resolved Hide resolved

modules/carto/src/basemap.ts Outdated Show resolved Hide resolved

zbigg force-pushed the zbigg/carto-fetchmap-custombasemaps branch from cbc6838 to 9291c3f Compare May 7, 2024 07:33

zbigg requested a review from donmccurdy May 7, 2024 07:33

felixpalmer reviewed May 7, 2024

View reviewed changes

zbigg requested a review from felixpalmer May 8, 2024 12:37

zbigg force-pushed the zbigg/carto-fetchmap-custombasemaps branch from de1954c to b1eec58 Compare May 8, 2024 14:40

donmccurdy approved these changes May 8, 2024

View reviewed changes

donmccurdy reviewed May 9, 2024

View reviewed changes

modules/carto/src/api/basemap.ts Outdated Show resolved Hide resolved

zbigg force-pushed the zbigg/carto-fetchmap-custombasemaps branch 3 times, most recently from 8171205 to bf1d5e5 Compare May 10, 2024 09:19

zbigg force-pushed the zbigg/carto-fetchmap-custombasemaps branch 5 times, most recently from f37124c to 0fda237 Compare May 13, 2024 15:35

donmccurdy approved these changes May 13, 2024

View reviewed changes

zbigg force-pushed the zbigg/carto-fetchmap-custombasemaps branch 3 times, most recently from 4c0969b to bf40a2a Compare May 13, 2024 16:25

zbigg added 17 commits May 14, 2024 14:56

Carto: fetchMap to support custom basemaps

a3c5bb2

add attribution in test

57be34e

attribution support

962db78

tileSize is part of basemap object in keplerMapConfig

976d692

fetchMap basemap returns style as it's either style or url

057f534

keplerMapConfig contains whole style

cf93623

cleanup / self-review

bdf58e6

support visibleLayerGroups in carto styles

a5a368d

cleanup / CR suggestions

796a3dd

use errorContext

fae0f75

tests for api/basemap

37f0147

fetchMap/mapStyle: add deprecation warning

c5689fa

carto: basemap support for google maps

4c280da

cleanup + docs

c64e8f3

carto: test app support for google basemaps

b73ed0f

docs

0ebe9c8

CR: typos and docs clarifications

1be5fc6

zbigg force-pushed the zbigg/carto-fetchmap-custombasemaps branch from bf40a2a to 1be5fc6 Compare May 14, 2024 12:56

donmccurdy merged commit 341b753 into master May 14, 2024
4 checks passed

donmccurdy deleted the zbigg/carto-fetchmap-custombasemaps branch May 14, 2024 13:04

donmccurdy pushed a commit that referenced this pull request May 23, 2024

feat(carto): Support custom basemaps in fetchMap (#8856)

af25b3e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Carto: fetchMap to support custom basemaps #8856

Carto: fetchMap to support custom basemaps #8856

zbigg commented Apr 30, 2024 •

edited

felixpalmer Apr 30, 2024

zbigg Apr 30, 2024

zbigg May 6, 2024

coveralls commented May 6, 2024 •

edited

zbigg May 6, 2024

srtena May 6, 2024

felixpalmer May 7, 2024

zbigg May 8, 2024

donmccurdy May 8, 2024

zbigg May 8, 2024

zbigg May 9, 2024 •

edited

donmccurdy left a comment

donmccurdy May 8, 2024

donmccurdy left a comment

		@@ -90,9 +90,13 @@ When the map was last updated.

		The [view state](../../developer-guide/views.md#view-state).

		#### `mapStyle` (string) {#mapstyle}

Carto: fetchMap to support custom basemaps #8856

Carto: fetchMap to support custom basemaps #8856

Conversation

zbigg commented Apr 30, 2024 • edited

Change List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbigg May 9, 2024 • edited

Choose a reason for hiding this comment

donmccurdy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donmccurdy left a comment

Choose a reason for hiding this comment

zbigg commented Apr 30, 2024 •

edited

coveralls commented May 6, 2024 •

edited

zbigg May 9, 2024 •

edited