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

viewState `padding` expected to be optional #2133

Open chrisgervang opened 1 year ago

chrisgervang commented 1 year ago

Currently I'm experiencing a Property 'padding' is missing in type ... but required in type 'ViewState'. error. I'd expect this type to be optional based on this implementation: https://github.com/visgl/react-map-gl/blob/109c334bdffaa5b2016c50bf6aaa119ce56082ce/src/mapbox/mapbox.ts#L544-L546

I believe the issue is in this type definition, which should define padding as optional.

https://github.com/visgl/react-map-gl/blob/109c334bdffaa5b2016c50bf6aaa119ce56082ce/src/types/external.ts#L30

I can open a PR to fix this if it should be optional.

Pessimistress commented 1 year ago

What is the use case? You are not supposed to supply viewState manually.

chrisgervang commented 1 year ago

I'm making a component that wraps Map and passes along any map props, so I thought to type this prop with MapProps like this:

import Map, {MapProps} from 'react-map-gl';

function Wrapper({mapProps}: {mapProps: MapProps}) {
  return (
    <Map
      {...mapProps}
    />
  )
}

I didn't realize that wasn't the intended usage since viewState is on the MapProps type, so TS doesn't hint this isn't the way to do it. I just looked and saw the examples spread viewState on as individual props, and I see that in the type definition as well. I'll do that.

Should viewState be removed from the MapProps type? i.e. MapProps = Omit<MapboxProps, 'viewState'> ...

References:

https://github.com/visgl/react-map-gl/blob/109c334bdffaa5b2016c50bf6aaa119ce56082ce/src/components/map.tsx#L27

https://github.com/visgl/react-map-gl/blob/109c334bdffaa5b2016c50bf6aaa119ce56082ce/src/mapbox/mapbox.ts#L246

Pessimistress commented 1 year ago

Yeah that's probably a good idea. I don't think deck.gl depends on react-map-gl types so it should be ok there.

chrisgervang commented 1 year ago

Do you know where this external controller use case is in use? I thought this was removed in v7 so that the map could only control deck, rather than the other way around. https://github.com/visgl/react-map-gl/blob/109c334bdffaa5b2016c50bf6aaa119ce56082ce/src/mapbox/mapbox.ts#L245-L249