aminghadersohi commented on code in PR #40804:
URL: https://github.com/apache/superset/pull/40804#discussion_r3376253283
##########
superset-frontend/plugins/preset-chart-deckgl/src/utilities/Shared_DeckGL.tsx:
##########
@@ -40,15 +48,23 @@ import {
isColorSchemeTypeVisible,
} from './utils';
import { TooltipTemplateControl } from './TooltipTemplateControl';
+import { hasMapboxApiKey } from '../utils/mapbox';
const categoricalSchemeRegistry = getCategoricalSchemeRegistry();
const sequentialSchemeRegistry = getSequentialSchemeRegistry();
export const DEFAULT_DECKGL_COLOR = { r: 158, g: 158, b: 158, a: 1 };
-let deckglTiles: string[][];
+type DeckGLTileChoice = [string, string];
+type MapStyleVisibilityProps = {
+ controls?: {
+ map_renderer?: { value?: unknown };
+ };
+};
+
+let deckglTiles: DeckGLTileChoice[];
Review Comment:
`deckglTiles` is a module-level mutable singleton: once `getDeckGLTiles()`
is called for the first time it is frozen for the module lifetime —
`jest.resetModules()` on *other* modules does not clear it. Tests that load
this module first (even transitively) silently fix the tile choices for all
later tests in the same worker. The new tests in `Shared_DeckGL.test.ts`
carefully work around this by dynamically importing after
`jest.resetModules()`, but the pattern will bite the next contributor who adds
a test. Consider exporting a `__resetDeckGLTilesForTesting` guard or removing
the cache entirely (bootstrap reads are cheap).
##########
superset-frontend/plugins/preset-chart-deckgl/src/utilities/Shared_DeckGL.tsx:
##########
@@ -456,15 +507,25 @@ export const mapProvider = {
label: t('Map Renderer'),
clearable: false,
renderTrigger: true,
- choices: [
- ['maplibre', t('MapLibre (open-source)')],
- ['mapbox', t('Mapbox (API key required)')],
- ],
- default: 'maplibre',
+ options: getLabeledMapRendererOptions({
+ hasMapboxKey: hasMapboxApiKey(),
+ }),
+ default: getDefaultMapRenderer(),
Review Comment:
`getDefaultMapRenderer()` reads `document.getElementById('app')` at module
import time and bakes the result as the permanent `default`. In jest/jsdom,
modules may be imported before the bootstrap element is present, making this
silently return `'maplibre'` regardless of `DEFAULT_MAP_RENDERER` config. The
tests in `Shared_DeckGL.test.ts` already demonstrate this by requiring
`jest.resetModules()` + dynamic `import()` just to test `config.default`
correctly. Consider moving the default resolution into `mapStateToProps` so it
is evaluated at render time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
