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: error on missing zoom or center #308

Merged
merged 15 commits into from
Apr 18, 2024

Conversation

maciej-ka
Copy link
Contributor

@maciej-ka maciej-ka commented Apr 10, 2024

Issue #283

  • console error on undefined zoom or center info
  • update existing tests on missing zoom/center info
  • rename to MapCameraState
  • refactor: move checking to useMapInstance()
  • test case which detects error

@maciej-ka maciej-ka marked this pull request as draft April 10, 2024 18:50

/**
* Props for the Google Maps Map Component
*/
export type MapProps = google.maps.MapOptions &
MapEventProps &
MapCameraProps &
Copy link
Contributor Author

@maciej-ka maciej-ka Apr 10, 2024

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.

Copy link
Collaborator

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:

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?

Copy link
Contributor Author

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.

@maciej-ka maciej-ka changed the title feat: require initZoom and initCenter feat: ts require zoom and center info Apr 10, 2024
@@ -35,7 +35,9 @@ export function useMapInstance(
const {
id,
defaultBounds,
// @ts-expect-error TS complains that it's not defined in MapProps
defaultCenter,
Copy link
Contributor Author

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.

Copy link
Collaborator

@usefulthink usefulthink Apr 11, 2024

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}.

Copy link
Collaborator

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?

Copy link
Collaborator

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.

Copy link
Contributor Author

@maciej-ka maciej-ka Apr 11, 2024

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.

Copy link
Collaborator

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

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.

@usefulthink
Copy link
Collaborator

Thanks again for your initiative!
One thing I would love to see added is a warning or error-message triggered in useMapInstance() when attempting to create a map without valid center/zoom for JS users.

@maciej-ka
Copy link
Contributor Author

Thanks a lot for guidance and codepens. The console error is ready here.
In TS Controlled/Uncontrolled sounds like a way to go, but perhaps in separate PR?

@maciej-ka maciej-ka marked this pull request as ready for review April 11, 2024 21:43
@maciej-ka maciej-ka changed the title feat: ts require zoom and center info feat: error on missing zoom or center Apr 11, 2024
@usefulthink
Copy link
Collaborator

I changed a few things around, I hope you don't mind. Didn't want to bother you with this...

  • I reverted the change of the MapCameraProps to MapCameraState, I remembered why it was called that way to begin with, it's the type you can use if you want to store the prop-values related to the camera in a 'controlled component' type of situation. So I feel MapCameraProps is a fitting name.
  • I changed the behaviour from throwing an exception to logging a warning as discussed in [Bug] missing center/zoom props don't produce an error or warning #283, the map will now render something even without any props specified. I feel like this will be the best developer experience.
  • There's another case we didn't look at, and that is providing defaultBounds instead of center/zoom values. Added support for that as well

@usefulthink usefulthink merged commit b318d67 into visgl:main Apr 18, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants