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.

Sign up for GitHub

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

Jump to bottom

feat: error on missing zoom or center #308

Merged
merged 15 commits into from
Apr 18, 2024
Merged

feat: error on missing zoom or center #308

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
64fd5b5
feat: handle missing center or zoom prop
maciej-ka Apr 10, 2024
901254f
feat: type require either zoom or initZoom, same for center
maciej-ka Apr 10, 2024
379ccfa
fix: remove duplicate definition
maciej-ka Apr 10, 2024
b35e75f
test: add madeup zoom and center to couple of test cases
maciej-ka Apr 10, 2024
31d4d85
feat: skip ts check failing for strange reason
maciej-ka Apr 10, 2024
56928ef
feat: revert changes in types
maciej-ka Apr 11, 2024
af61261
feat: error on undefined center or zoom prop
maciej-ka Apr 11, 2024
9925d7e
refactor: rename to MapCameraState
maciej-ka Apr 11, 2024
15ded50
refactor: move check to use-map-instance
maciej-ka Apr 11, 2024
933f154
Merge branch 'main' into 283-error-on-missing-center-or-zoom
maciej-ka Apr 11, 2024
a28747d
test: check that error is present when info is missing
maciej-ka Apr 11, 2024
17cf95f
Merge branch 'main' into 283-error-on-missing-center-or-zoom
maciej-ka Apr 13, 2024
0753f71
revert renaming MapCameraProps to MapCameraState
usefulthink Apr 18, 2024
02a9cdc
fix: log a warning instead of throwing an error, make sure the map re…
usefulthink Apr 18, 2024
000afc9
fix: avoid typescript equals import
usefulthink Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions src/components/__tests__/map.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,17 +56,21 @@ afterEach(() => {
test('map instance is created after api is loaded', async () => {
mockContextValue.status = APILoadingStatus.LOADING;

const {rerender} = render(<GoogleMap />, {wrapper});
const {rerender} = render(
<GoogleMap zoom={8} center={{lat: 53.55, lng: 10.05}} />,
{wrapper}
);

expect(createMapSpy).not.toHaveBeenCalled();

// rerender after loading completes
mockContextValue.status = APILoadingStatus.LOADED;
rerender(<GoogleMap />);
rerender(<GoogleMap zoom={8} center={{lat: 53.55, lng: 10.05}} />);
expect(createMapSpy).toHaveBeenCalled();
});

test("map is registered as 'default' when no id is specified", () => {
render(<GoogleMap />, {wrapper});
render(<GoogleMap zoom={8} center={{lat: 53.55, lng: 10.05}} />, {wrapper});

expect(mockContextValue.addMapInstance).toHaveBeenCalledWith(
mockInstances.get(google.maps.Map).at(-1),
Expand All @@ -79,7 +83,9 @@ test('throws an exception when rendering outside API provider', () => {
jest.spyOn(console, 'error').mockImplementation(() => {});

// render without wrapper
expect(() => render(<GoogleMap />)).toThrowErrorMatchingSnapshot();
expect(() =>
render(<GoogleMap zoom={8} center={{lat: 53.55, lng: 10.05}} />)
).toThrowErrorMatchingSnapshot();
});

describe('creating and updating map instance', () => {
Expand Down
11 changes: 6 additions & 5 deletions src/components/map/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,20 @@ export type {
} from './use-map-events';

export type MapCameraProps = {
center: google.maps.LatLngLiteral;
zoom: number;
heading?: number;
tilt?: number;
};
} & (
| {center: google.maps.LatLngLiteral}
| {defaultCenter: google.maps.LatLngLiteral}
) &
({zoom: number} | {defaultZoom: number});

/**
* 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 •
edited
Loading

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:

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?

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.

DeckGlCompatProps & {
/**
* An id for the map, this is required when multiple maps are present
Expand All @@ -67,8 +70,6 @@ export type MapProps = google.maps.MapOptions &
*/
controlled?: boolean;

defaultCenter?: google.maps.LatLngLiteral;
defaultZoom?: number;
defaultHeading?: number;
defaultTilt?: number;
/**
Expand Down
2 changes: 2 additions & 0 deletions src/components/map/use-map-instance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

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 •
edited
Loading

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 •
edited
Loading

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

// @ts-expect-error TS complains that it's not defined in MapProps
defaultZoom,
defaultHeading,
defaultTilt,
Expand Down
4 changes: 3 additions & 1 deletion src/hooks/__tests__/use-map.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@ test('returns the parent map instance when called without id', async () => {
// Create wrapper component
const wrapper = ({children}: React.PropsWithChildren) => (
<MockApiContextProvider>
<GoogleMap>{children}</GoogleMap>
<GoogleMap zoom={8} center={{lat: 53.55, lng: 10.05}}>
{children}
</GoogleMap>
</MockApiContextProvider>
);

Expand Down
Loading