visgl / react-map-gl

React friendly API wrapper around MapboxGL JS
http://visgl.github.io/react-map-gl/
Other
7.88k stars 1.35k forks source link

[Feat] upgrade to fit `mapbox-gl@3.5.0` #2411

Open kiruushaaa opened 4 months ago

kiruushaaa commented 4 months ago

Target Use Case

https://github.com/mapbox/mapbox-gl-js/issues/13203

Proposal

https://github.com/mapbox/mapbox-gl-js/issues/13203

Pessimistress commented 4 months ago

Thanks. Do you have any suggestion on how we should handle this? Do we just make @types/mapbox-gl an optional dependency?

kiruushaaa commented 4 months ago

Hi. I didn't touch anything under the hood both of your library or new types due to absence of time. Also you seem to be maintaining backwards compatibility, so there may be another task along with just the types package updating.

I'll check just running the latest version of both types and this library... or you'll do it faster 😁

jgarplind commented 4 months ago

I just tried to upgrade and ran into one incompatibility, but (perhaps obviously) it had no functional impact, just caused a type error.

Specifically, I have defined a few layers like so:

const zoneLayer: FillLayerSpecification = {
  id: "zone",
  type: "fill",
  paint: {
    "fill-color": "#2d4ad9",
    "fill-opacity": 0.3,
  },
};

This Layer is then wrapped in a Source before being rendered, like so:

<Source data={zoneData} type="geojson">
    <Layer {...zoneLayer} />
</Source>

With the new types however, I need to define a source: string as part of my Layer specification:

Property '"source"' is missing in type '{ id: string; type: "fill"; paint: { "fill-color": string; "fill-opacity": number; }; }' but required in type 'FillLayerSpecification'.ts(2741)

Adding a random string such as source: "TODO" removes the type error, and retains functionality.

My assumption is that this library uses mechanisms that renders the source field unnecessary, but that is obviously not something that the main library is aware of.

Pessimistress commented 4 months ago

@jgarplind I don't think there is an export called FillLayerSpecification.

jgarplind commented 4 months ago

@Pessimistress I think there is since the recent 3.5.0 release that this issue is about.

The migration guide indicates it:

Style-Spec JSON objects in GL JS are suffixed with *Specification (e.g., StyleSpecification, LayerSpecification)

and other than source, it seems to fit well as a successor of FillLayer from @types/mapbox-gl.

Pessimistress commented 4 months ago

Oh, you are importing it directly from mapbox-gl. The Layer component does not require the source prop. You can import LayerProps from react-map-gl instead, though it is currently based on @types/mapbox-gl not the official types.

jgarplind commented 4 months ago

That's right. Thanks for clarifying!

I based my practice on your documentation, though I must have confused mapbox-gl types with the react-map-gl re-exported types. However, even using react-map-gl re-exported types, the same issue remains.

So if you think it is preferable to use the LayerProps over the XLayer types, would you be open for a PR updating the Layer documentation accordingly?

FullstackWEB-developer commented 4 months ago

This error occurred when using @mapbox/mapbox-gl-draw after upgrading mapbox-gl to 3.5.1.

Type 'MapboxDraw' does not satisfy the constraint 'IControl<MapInstance>'.
  Types of property 'onAdd' are incompatible.
    Type '(map: Map$1) => HTMLElement' is not assignable to type '(map: MapInstance) => HTMLElement'.
      Types of parameters 'map' and 'map' are incompatible.
        Type 'MapInstance' is missing the following properties from type 'Map$1': style, painter, handlers, _container, and 267 more.ts(2344)

this is DrawControl.ts file:

import MapboxDraw from '@mapbox/mapbox-gl-draw';
import { useControl } from 'react-map-gl';

import type { ControlPosition } from 'react-map-gl';
import type { MapContextValue } from 'react-map-gl/dist/esm/components/map';

import drawStyles from './draw-modes/draw-style';

type DrawControlProps = ConstructorParameters<typeof MapboxDraw>[0] & {
  position?: ControlPosition;

  onCreate: (evt: { features: object[] }) => void;
  onUpdate: (evt: { features: object[]; action: string }) => void;
  onDelete: (evt: any) => void;
};

export default function DrawControl(props: DrawControlProps) {
  useControl<MapboxDraw>(
    () => new MapboxDraw({
      ...props,
      styles: drawStyles,
    }),
    ({ map }: MapContextValue) => {
      map.on('draw.create', props.onCreate);
      map.on('draw.update', props.onUpdate);
      map.on('draw.delete', props.onDelete);
    },
    ({ map }: MapContextValue) => {
      map.off('draw.create', props.onCreate);
      map.off('draw.update', props.onUpdate);
      map.off('draw.delete', props.onDelete);
    },
    {
      position: props.position
    }
  );

  return null;
}

DrawControl.defaultProps = {
  onCreate: () => { },
  onUpdate: () => { },
  onDelete: () => { }
};
JessiDeuxTrois commented 3 months ago

I’m getting the same errors as the above with using the draw control code example. Are there fixes for this?

FullstackWEB-developer commented 2 months ago

I’m getting the same errors as the above with using the draw control code example. Are there fixes for this?

@JessiDeuxTrois I am using mapbox-gl by downgrading it to 3.4.0.

tannera commented 3 weeks ago

@Pessimistress, can you estimate the effort in supporting the latest mapbox version (3.5.0 and beyond)? It seems there are typing issues https://github.com/mapbox/mapbox-gl-js/issues/13203