Moving all the map-related code to TypeScript #100

richterb · 2022-08-02T14:52:45Z

No description provided.

…d to Choropleth only

etienneburdet

Really nice, it will help a ton 😍 I have a few comments, but overall it's working great!

The any parts are having me wondering if we should have a no-explicit-any rule. On the platform, it's a pain, but on a fresh codebase like this one it would be a nice reminder IMO (although it's easy enough).

I really like the comments on types (didn't know they show in the IDE!), we should enforce that more in our code I think.

As I said, we should not forget to import the new Colorscale types in the platform too.

etienneburdet · 2022-08-16T07:59:19Z

packages/visualizations/src/components/Map/types.ts

+    label: string;
+    /** Value of the key used to match shapes and numeric data */
+    key?: string;
+}) => string;


The return type can be auto inferred from the function and some might prefer typying the parameters for reuse? Imo it doesn't hurt to be explicit like this and I really don't mind it.

My reasoning here is that the SDK is exposing an option of this type, and typing even the return value makes sure that if the user passes a function that returns an int or a JSX.Element, their compiler/IDE complains that the SDK expects a function that always returns a string.

etienneburdet · 2022-08-16T09:45:17Z

packages/visualizations/src/components/Map/types.ts

+
+export interface ChoroplethOptions {
+    shapes: ChoroplethShapeValue;
+    colorsScale?: ColorsScale;


More a reminder: at some point we have to import those into the platform as well (and then derive/export what we need). Right now we have definitions on both sides
https://github.com/opendatasoft/platform/blob/develop/ods/react/src/components/PageRenderer/typeRenderers/map/types.ts

As discussed, there is a draft PR on the platform repo for that https://github.com/opendatasoft/platform/pull/9571, which makes the code nicer and tighter indeed

etienneburdet · 2022-08-16T09:47:47Z

packages/visualizations/src/components/Map/types.ts

+
+export type MapRenderTooltipFunction = (f: Feature) => string;
+
+export type ChoroplethLayer = Omit<FillLayerSpecification, 'id' | 'source'>;


Learned something 😍

etienneburdet · 2022-08-16T10:03:45Z

packages/visualizations/src/components/Map/MapRender.svelte

    // bounding box to start from, and restrict to it
-    export let bbox;
-
+    export let bbox: BBox;


Do we always want a BBox here or is LngLatBoundsLikeacceptable as well? Or maybe it's just never the case. I am unsure since we accept [LngLatLike, LngLatLike] in all functions after if I am correct.

I was a bit torn here initially, because LngLatBoundsLike comes from Maplibre, and I'd rather expose things that are generic. But overall I think I prefer to require a parameter in a specific non-ambiguous format instead of tolerating many and having to support and sometimes transform them, and the bbox one is the most straight-forward ([number, number, number, number]).

etienneburdet · 2022-08-16T10:09:51Z

packages/visualizations/src/components/Map/MapRender.svelte

-    export let colorsScale;
-    export let dataBounds;
-    let clientWidth;
-    $: legendVariant = clientWidth <= 375 ? 'fluid' : 'fixed';


Could maybe be constants and then we use the […] as const; … = typeof array[number] trick? So we have only one source for the constants (although right now it's not really overwhelming).

Indeed I can do something like

const LEGENDVARIANT_FLUID = 'fluid'; const LEGENDVARIANT_FIXED = 'fixed'; export type LegendVariant = typeof LEGENDVARIANT_FLUID | typeof LEGENDVARIANT_FIXED;

But I'm not sure of the best place where to put the constants. I'd like to put them into ColorLegend.svelte, but I'm not sure how imports will work, and also this would be a circular import quickly.

What's the trick you're talking about exactly? Maybe it's precisely the answer to my problems, but I don't think I know about it

etienneburdet · 2022-08-16T13:29:36Z

packages/visualizations/src/components/Map/types.ts

+
+export type ChoroplethLayer = Omit<FillLayerSpecification, 'id' | 'source'>;
+
+export type MapLayer = ChoroplethLayer;


Since it's laying ground for future development, maybe we could do Partial<FillLayerSpecification> (or having only certain fiels as optional) and then have more specific map types (right now only Choropleth) be more restrictive if need be. In case someone need a layer type that's not from us? Maybe its too far fetched though and MapLayer = ChoroplethLayer | PointerMapLayer | … will be enough 🤷

In the code right now, we're literally just adding our own id and source and expecting the result to be a FillLayerSpecification, so this fits exactly the code and the intent, and if Maplibre evolves its specification, it's going to break instantly, which is nice. Right now I'm expecting that we won't have a lot of supported types of layer, but it really depends on how MapRender evolves. My experiences with Maplibre in general encourage me to really strenghten the configuration when we can 😅

etienneburdet · 2022-08-16T13:42:55Z

packages/visualizations/src/components/Map/utils.ts

-        const thresholdArray = [];
-        colorsScale.colors.forEach((_color, i) => {
+        const thresholdArray: number[] = [];
+        colorsScale.colors.forEach((_color, i: number) => {


i Can be auto-infered safely I think (at least it works 😅 )

Absolutely, I'm not sure why I went overboard here but not even with _color 😁

etienneburdet · 2022-08-16T14:25:07Z

packages/visualizations/src/components/Map/utils.ts

@@ -36,15 +49,15 @@ export const colorShapes = (geoJson, values, colorsScale, emptyValueColor) => {
        scale = chroma.scale([colorMin, colorMax]).domain([min, max]);
    }

-    const dataMapping = {};
+    const dataMapping: any = {};


Probably better with a reduce and a map type:

Suggested change

const dataMapping: any = {};

type ChoroPlethDataMapping = {

[key: ChoroplethDatavlue['x']]: ChoroplethDataVlue['y'];

}

const dataMapping: ChoroPlethDataMapping = values.reduce( (mapping, v) => mapping[v.x] = v.y, {});

(just the idea, I'm not sure it's fully working 😛 )

I tried the reduce, but it ends up being a bit more verbose because the initial value {} also needs the same type etc.
But I completely missed that I could have an object where both the key and value can be typed, so I replaced the any with the inlined type you suggested!

etienneburdet · 2022-08-16T14:30:07Z

packages/visualizations/src/components/Map/utils.ts

-function mergeBboxFromFeaturesWithSameKey(features) {
-    const mergedBboxes = {};
+function mergeBboxFromFeaturesWithSameKey(features: Feature[]) {
+    const mergedBboxes: any = {};


Same here, I would use a reduce with a stricter type.

Same as above, I didn't end up using a reduce because it complicated the code IMO, but I still replaced the any with a real inline type.

etienneburdet · 2022-08-16T15:12:09Z

packages/visualizations/src/components/Map/utils.ts

@@ -180,5 +183,5 @@ export const getFixedTooltips = (shapeKeys, features, renderTooltip) => {
        return null;
    });

-    return popups;
+    return popups.filter(Boolean) as ChoroplethFixedTooltipDescription[];


So apparently Boolean doesn't have the right signature for TS. I really like the isPresent solution, although we could craft our own function easily too.

Whatever the solution, I think we should do the extra effort to popus.filter<ChoroplethFixedTooltipDescription>(…) rather than hard casting. But this bug is a bit of pain indeed…

I spent a lot of time trying to do it the right way, but somehow didn't succeed. I'll try using compact from lodash which should do the same thing, and has types that should work here.

If we don't want to use the npm package ts-is-present, the other solution seems to be using type guard returns:

function nonNullable<T>(value: T): value is NonNullable<T> { return value !== null && value !== undefined; } return popus.filter<ChoroplethPopups>(nonNullable)

I used an inline function:

return popups.filter((item): item is NonNullable<ChoroplethFixedTooltipDescription> => Boolean(item)) as ChoroplethFixedTooltipDescription[];

Thank you for teaching us about is 😁

wmai

Very nice! Thanks @richterb 🙏

To be honest, I didn't want to delay this PR so I did a somewhat superficial review here. Maps are still a bit confusing for me, but I intend to dive into it and really understand what's going on.

packages/visualizations/src/components/Map/MapRender.svelte

packages/visualizations/src/components/Map/utils.ts

packages/visualizations/src/components/Map/MapRender.svelte

wmai

LGTM 👏

wmai · 2022-08-23T17:21:59Z

packages/visualizations/src/components/Map/Choropleth.svelte

    import turfBbox from '@turf/bbox';
+    import type { SourceSpecification } from 'maplibre-gl';
+    // eslint-disable-next-line import/no-unresolved


Regarding this issue (type only imports), I seems that the version 2.25.2 of eslint-plugin-import fixed it. I've tested this version and no more linter error 👍.

Here's the PR: import-js/eslint-plugin-import#2220

(same thing in MapRender.svelte line 16)

Rewrite all map-related code to TypeScript

023beb3

richterb force-pushed the map-typescript branch from cb92bf0 to 023beb3 Compare August 4, 2022 09:40

richterb marked this pull request as ready for review August 4, 2022 09:40

richterb added 2 commits August 4, 2022 14:29

Document the less trivial types

d5361b5

Keep generic types for MapRender, as it should be generic and not tie…

4ec004c

…d to Choropleth only

wmai self-requested a review August 5, 2022 10:38

etienneburdet requested changes Aug 16, 2022

View reviewed changes

richterb added 2 commits August 19, 2022 09:58

Improved types after PR review

03197e9

Linting

2a0a57e

wmai reviewed Aug 19, 2022

View reviewed changes

packages/visualizations/src/components/Map/MapRender.svelte Outdated Show resolved Hide resolved

packages/visualizations/src/components/Map/MapRender.svelte Outdated Show resolved Hide resolved

packages/visualizations/src/components/Map/utils.ts Outdated Show resolved Hide resolved

Improve typing after PR review

96d7281

wmai reviewed Aug 19, 2022

View reviewed changes

packages/visualizations/src/components/Map/MapRender.svelte Outdated Show resolved Hide resolved

Replace ts-ignore with a simple value check

20727d3

richterb requested review from wmai and etienneburdet August 22, 2022 08:04

wmai approved these changes Aug 22, 2022

View reviewed changes

wmai reviewed Aug 23, 2022

View reviewed changes

richterb merged commit baaf481 into main Aug 24, 2022

richterb deleted the map-typescript branch August 24, 2022 10:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving all the map-related code to TypeScript #100

Moving all the map-related code to TypeScript #100

richterb commented Aug 2, 2022

etienneburdet left a comment •

edited

Loading

etienneburdet Aug 16, 2022

richterb Aug 19, 2022

etienneburdet Aug 16, 2022

richterb Aug 19, 2022

etienneburdet Aug 16, 2022

etienneburdet Aug 16, 2022

richterb Aug 19, 2022

etienneburdet Aug 16, 2022

richterb Aug 19, 2022 •

edited

Loading

etienneburdet Aug 16, 2022

richterb Aug 19, 2022

etienneburdet Aug 16, 2022

richterb Aug 19, 2022

etienneburdet Aug 16, 2022

richterb Aug 19, 2022

etienneburdet Aug 16, 2022

richterb Aug 19, 2022

etienneburdet Aug 16, 2022

richterb Aug 16, 2022

etienneburdet Aug 17, 2022

richterb Aug 19, 2022

wmai left a comment

wmai left a comment

wmai Aug 23, 2022 •

edited

Loading


		export type MapRenderTooltipFunction = (f: Feature) => string;

		export type ChoroplethLayer = Omit<FillLayerSpecification, 'id' \| 'source'>;


		export type ChoroplethLayer = Omit<FillLayerSpecification, 'id' \| 'source'>;

		export type MapLayer = ChoroplethLayer;

-    const dataMapping: any = {};
+    type  ChoroPlethDataMapping = {
+      [key: ChoroplethDatavlue['x']]: ChoroplethDataVlue['y'];
+    }
+    const dataMapping: ChoroPlethDataMapping = values.reduce( (mapping, v) => mapping[v.x] = v.y, {});

Moving all the map-related code to TypeScript #100

Moving all the map-related code to TypeScript #100

Conversation

richterb commented Aug 2, 2022

etienneburdet left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richterb Aug 19, 2022 • edited Loading

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

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

Choose a reason for hiding this comment

wmai left a comment

Choose a reason for hiding this comment

wmai left a comment

Choose a reason for hiding this comment

wmai Aug 23, 2022 • edited Loading

Choose a reason for hiding this comment

etienneburdet left a comment •

edited

Loading

richterb Aug 19, 2022 •

edited

Loading

wmai Aug 23, 2022 •

edited

Loading