Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feat] upgrade to fit [email protected] #2411

Open
kiruushaaa opened this issue Jul 5, 2024 · 11 comments
Open

[Feat] upgrade to fit [email protected] #2411

kiruushaaa opened this issue Jul 5, 2024 · 11 comments
Labels

Comments

@kiruushaaa
Copy link

Target Use Case

mapbox/mapbox-gl-js#13203

Proposal

mapbox/mapbox-gl-js#13203

@Pessimistress
Copy link
Collaborator

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

@kiruushaaa
Copy link
Author

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
Copy link

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
Copy link
Collaborator

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

@jgarplind
Copy link

@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
Copy link
Collaborator

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
Copy link

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
Copy link

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
Copy link

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

@FullstackWEB-developer
Copy link

FullstackWEB-developer commented Sep 4, 2024

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
Copy link

tannera commented Nov 5, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants