-
Notifications
You must be signed in to change notification settings - Fork 95
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: error on missing zoom or center #308
feat: error on missing zoom or center #308
Conversation
src/components/map/index.tsx
Outdated
|
||
/** | ||
* Props for the Google Maps Map Component | ||
*/ | ||
export type MapProps = google.maps.MapOptions & | ||
MapEventProps & | ||
MapCameraProps & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like inclusion of MapCameraProps
is missing? Without it zoom
and center
are not a Map property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zoom
and center
are defined in google.maps.MapOptions
so they haven't been repeated here.
MapCameraProps
isn't included since it is intended to be used together with the MapCameraChangedEvent
(it's a type you can use for the details of that event), see here for example:
react-google-maps/examples/multiple-maps/src/app.tsx
Lines 33 to 41 in 9618024
const [cameraState, setCameraState] = | |
useState<MapCameraProps>(INITIAL_CAMERA_STATE); | |
// we only want to receive cameraChanged events from the map the | |
// user is interacting with: | |
const [activeMap, setActiveMap] = useState(1); | |
const handleCameraChange = useCallback((ev: MapCameraChangedEvent) => { | |
setCameraState(ev.detail); | |
}, []); |
Maybe it should be renamed to MapCameraState
to avoid confusion with the props?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I would add that rename.
@@ -35,7 +35,9 @@ export function useMapInstance( | |||
const { | |||
id, | |||
defaultBounds, | |||
// @ts-expect-error TS complains that it's not defined in MapProps | |||
defaultCenter, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why TS thinks defaultCenter is not defined here? ... when testing on pojo typed as MapProps that prop will be there, as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recreated a simplified version of the situation in ts-playground.
The error message "Property 'defaultCenter' does not exist on type 'MapProps'.", is a bit misleading, but its true: You're using union-types, and those only contain the fields that are present in all variations of the union (in case of {center: X} | {defaultCenter: X}
that is the empty object). The center prop works since it's already defined in the google.maps.MapOptions
type.
What would work here is to specify the union as {center: LatLng, defaultCenter?: LatLng} | {center?: LatLng, defaultCenter: LatLng}
, see here. The union type then resolves to {center?:LatLng, defaultCenter?:LatLng}
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I have to say, I'm not a big fan of the TS error-message it will give you if you leave either away.
Argument of type '{ foo: number; }' is not assignable to parameter of type 'MapProps'.
Type '{ foo: number; }' is not assignable to type 'MapOptions & { center?: LatLng | undefined; defaultCenter: LatLng; }'.
Property 'defaultCenter' is missing in type '{ foo: number; }' but required in type '{ center?: LatLng | undefined; defaultCenter: LatLng; }'.(2345)
Not sure if that is easier to use than a console.warn
message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an alternative that produces slightly more readable error-messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems it's a puzzle if TS can report this error as "either center or defaultCenter has to be provided", so far warnings had only one of two prop names. I think I will switch to console.warn solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can go with the last option, I like the Idea of being able to statically warn about missing required props, even if the error-message is a bit clunky. I'll try a few more things to see if it gets any better...
@@ -35,7 +35,9 @@ export function useMapInstance( | |||
const { | |||
id, | |||
defaultBounds, | |||
// @ts-expect-error TS complains that it's not defined in MapProps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't commit those – there's nothing wrong with tests failing in draft-PRs, and it's running risk of missing one of them at some point.
Thanks again for your initiative! |
Thanks a lot for guidance and codepens. The console error is ready here. |
I changed a few things around, I hope you don't mind. Didn't want to bother you with this...
|
Issue #283