Incorrect TS typings

[tirli]

Hi, I updated to the last version and got couple of new errors:

1. setPathOptions should have second argument as optional in TS definition. Also newly added merge field is missing
2. disableDraw accepts shape argument. It is optional, but it is still there. I guess the right option is to remove shape from the code, not from typings.
3. I see that there are newly added typings for event handlers. For example, the layer is now L.Layer. Are you sure this is the best typings? Because now I cannot use setStyle or toGeoJSON functions without fighting TS

[Falke-Design]

Thanks for reporting, we will change 1 and 2.

@ryan-morris can you please answer point 3.

[tirli]

One more addition I just found: rotateMode is missing in ToolbarOptions

[Falke-Design]

PR for the changes: #915

[ryan-morris]

@tirli do you mean like this, but before you didn't have to typecast polygon?

import { PM, Polygon } from 'leaflet';
export type DrawStartEventHandler = (e: {
  shape: PM.SUPPORTED_SHAPES;
  workingLayer: L.Layer;
}) => void;

const a: DrawStartEventHandler = e => {
  const geo = e.workingLayer.toGeoJSON(); // does not work
  const geo2 = (e.workingLayer as Polygon).toGeoJSON(); // works
};

@Falke-Design is there a specific subset of the types that can be passed into the event handlers as workingLayer? Unless all of the types that layer can be set to have a toGeoJSON() method, you would have to typecast to access more type specific methods

[tirli]

@ryan-morris yes. Before that, it was any, so I just assigned it to the variable with type L.Polygon, and it was perfectly fine. Though I guess L.Polygon is not an option here, we need something more universal than in my case, where it is always polygons. Probably some kind of generic?

[Falke-Design]

layer can only be one of the following types: Marker, CircleMarker, Circle, Polygon, Polyline, Rectangle, ImageOverlay and maybe LayerGroup. But it can also be a extended Version of a Layer type for example L.RotatebleMarker from a plugin.

And ImageOverlay don't have the function toGeoJSON()

[ryan-morris]

@Falke-Design one option would be to add back the SUPPORTED_SHAPE_TYPES I had here but use it for workingLayer and other L.Layer references. But based on your last message, typecasting would still be necessary since this still wouldn't work:

export type DrawStartEventHandler = (e: {
  shape: PM.SUPPORTED_SHAPES;
  workingLayer: L.Polygon|L.ImageOverlay;
}) => void;

const a: DrawStartEventHandler = e => {
  const geo = e.workingLayer.toGeoJSON(); // does not work
  const geo2 = (e.workingLayer as Polygon).toGeoJSON(); // works
};

[ryan-morris]

defining all possible variations may work...

interface DrawStartEventArgs {
  workingLayer: Layer,
  shape: PM.SUPPORTED_SHAPES
}

interface DrawStartEventArgsPolygon extends DrawStartEventArgs {
  workingLayer: Polygon,
  shape: 'Polygon'
};

export type DrawStartEventHandler = (e: DrawStartEventArgs) => void;

const a: DrawStartEventHandler = (e: DrawStartEventArgsPolygon) => {
  const geo = e.workingLayer.toGeoJSON(); // works
  const geo2 = (e.workingLayer as Polygon).toGeoJSON(); // works
};

[Falke-Design]

In my opinion it is valid to have a typecasting here. And an instanceof check is needed anyway and then it is working too:

const a: DrawStartEventHandler = e => {
  if(e.workingLayer instanceof L.Polygon) {
    const geo = e.workingLayer.toGeoJSON(); // does work
  }
};

[ryan-morris]

In my opinion it is valid to have a typecasting here. And an instanceof check is needed anyway and then it is working too:

I agree, type checking is probably the only safe implementation option. Even in this example just because a different argument type could be specified, doesn't mean that's what's going to come in. That's based on the event registration which afaik isn't possible to trace all the way through

[tirli]

instanceof is definitely helpful. And I can use it for now. And I like it more than type casting since it is checked in runtime.

I guess ideally there should be type guards based on SUPPORTED_SHAPE_TYPES.

[Falke-Design]

So we will now leave it as it is?

[tirli]

Depends on how much time are you willing to invest in this :)

instanceof works. But the abstraction is leaking. I need to know that you are using L.Polygon for shape: Polygon under the hood to make a proper check.

Ideally, I would like to write something like

const a: DrawStartEventHandler = e => {
  if (e.shape === 'Polygon') {
    const geo = e.workingLayer.toGeoJSON(); // does work because TS understands it is `L.Polygon`
  }
};

Most likely Discriminating unions will be enough.

type Event = { shape: 'Polygon', workingLayer: L.Polygon } | { shape: 'Polyline', workingLayer: L.Polyline }
type DrawStartEventHandler = (e: Event) => void;

[ryan-morris]

Because there are custom types allowed to come through as mentioned before, there is still ambiguity when trying to check just based on the shape, for example:

 export type ShapeAndLayerEventArgs = 
        | { shape: 'Polygon', layer: L.Polygon }
        | { shape: 'Circle', layer: L.Circle }
        | { shape: 'CircleMarker', layer: L.CircleMarker }
        | { shape: 'Cut', layer: L.Layer } // todo: type?
        | { shape: 'ImageOverlay', layer: L.ImageOverlay }
        | { shape: 'Rectangle', layer: L.Rectangle }
        | { shape: string, layer: L.Layer };

    export type ShapeAndWorkingLayerEventArgs = 
        | { shape: 'Polygon', workingLayer: L.Polygon }
        | { shape: 'Circle', workingLayer: L.Circle }
        | { shape: 'CircleMarker', workingLayer: L.CircleMarker }
        | { shape: 'Cut', workingLayer: L.Layer } // todo: type?
        | { shape: 'ImageOverlay', workingLayer: L.ImageOverlay }
        | { shape: 'Rectangle', workingLayer: L.Rectangle }
        | { shape: string, workingLayer: L.Layer };

    export type ShapeAndLayerAndWorkingLayerEventArgs = ShapeAndLayerEventArgs & ShapeAndWorkingLayerEventArgs;

export type DrawStartEventHandler = (e: ShapeAndLayerAndWorkingLayerEventArgs) => void;

If you try to do

    const d: PM.DrawStartEventHandler = e => {
      if(e.shape === 'Polygon') {
        const geoJson = e.layer.toGeoJSON();
      }
    }

It results in

Property 'toGeoJSON' does not exist on type 'Layer | Polygon<any>'.
  Property 'toGeoJSON' does not exist on type 'Layer'.ts(2339)

Thoughts? Doing { shape: string, layer: any } would let you do the toGeoJSON() but any is generally discouraged.

[tirli]

@ryan-morris According to the docs DrawStartEventHandler should only have workingLayer, not layer. So no need to say that pm:drawstart handler is ShapeAndLayerAndWorkingLayerEventArgs. It is only ShapeAndWorkingLayerEventArgs. Also, when you added shape: string discriminating union stopped working. Why do we need this part? It is possible to get some unknown shape type? I haven't found this in the code...

So I guess this should be a working solution.

export type ShapeAndWorkingLayerEventArgs =
  | { shape: 'Polygon', workingLayer: L.Polygon }
  | { shape: 'Circle', workingLayer: L.Circle }
  | { shape: 'CircleMarker', workingLayer: L.CircleMarker }
  | { shape: 'ImageOverlay', workingLayer: L.ImageOverlay }
  | { shape: 'Rectangle', workingLayer: L.Rectangle }

export type DrawStartEventHandler = (
  e: ShapeAndWorkingLayerEventArgs
) => void;

const d: DrawStartEventHandler = (e) => {
  if (e.shape === 'Polygon') {
    const geoJson = e.workingLayer.toGeoJSON();
  }
};

export type ShapeAndLayerEventArgs =
  | { shape: 'Polygon', layer: L.Polygon }
  | { shape: 'Circle', layer: L.Circle }
  | { shape: 'CircleMarker', layer: L.CircleMarker }
  | { shape: 'Cut', layer: L.Layer, originalLayer: L.Layer } // todo: type?
  | { shape: 'ImageOverlay', layer: L.ImageOverlay }
  | { shape: 'Rectangle', layer: L.Rectangle }

export type CreateEventHandler = (
  e: ShapeAndLayerEventArgs
) => void;

const c: CreateEventHandler = (e) => {
  if (e.shape === 'Polygon') {
    const geoJson = e.layer.toGeoJSON();
  }
};

Not sure what to do with the Cut, though. It should be only in layers (not workingLayer) union and also has originalLayer. Thought I'm not sure what typing is possible there. Probably L.Layer + instanceof is the best we can do for that specific case.

[Falke-Design]

With copyDrawControl() it is possible to create custom shapes, so it is needed to be "string".

What is with Line and Marker in ShapeAndWorkingLayerEventArgs and ShapeAndLayerEventArgs?