Fix typechecking (#1798)

[trumbitta]

Fixes a resurgence of the bug found in #1798

It might be a breaking change because it forces code like this:

  const draw = useControl<MapboxGlDraw>(
    () => new MapboxGlDraw(rest),
    ({ map }: MapContextValue<MapInstance>) => {
      map.on("draw.create", onCreate);
    },
    ({ map }: MapContextValue<MapInstance>) => {
      map.off("draw.create", onCreate);
    },
    {
      position,
    },
  );

to become:

  const draw = useControl<MapboxGlDraw>(
    () => new MapboxGlDraw(rest),
    ({ map }: MapContextValue<MapInstance>) => {
      map?.on("draw.create", onCreate);
    },
    ({ map }: MapContextValue<MapInstance>) => {
      map?.off("draw.create", onCreate);
    },
    {
      position,
    },
  );

[Pessimistress]

That does not look like the right solution. If you need to make something optional, you add | undefined to where you use it, not in its type definition.

Can you create a project that reproduces your issue?

[trumbitta]

That does not look like the right solution. If you need to make something optional, you add | undefined to where you use it, not in its type definition.

Can you create a project that reproduces your issue?

It's the same issue as #1798, although happening with plain tsc instead of Vite, and it's also the same fix as #1799

I'll try to put together a repro 🙏

[trumbitta]

@Pessimistress here's the repro!

https://codesandbox.io/p/sandbox/wispy-thunder-tx58mq

Run yarn tsc or yarn start if it doesn't start automatically.

[Pessimistress]

it's also the same fix as #1799

No, it's not the same fix. You need to change it where MapRef is used, not where it is defined.

Looks like TypeScript compiler is not inferring the return type of useMap correctly. You can enforce it like this:

--- a/src/components/use-map.tsx
+++ b/src/components/use-map.tsx
@@ -52,7 +52,10 @@ export const MapProvider: React.FC<{children?: React.ReactNode}> = props => {
   );
 };

-export function useMap<MapT extends MapInstance>() {
+export function useMap<MapT extends MapInstance>(): {
+  [id: string]: MapRef<MapT> | undefined;
+  current?: MapRef<MapT>;
+} {
   const maps = useContext(MountedMapsContext)?.maps;
   const currentMap = useContext(MapContext);

[trumbitta]

@Pessimistress It kind of makes sense, but I don't think I know how to test this kind of change in this repo.
My change I could test locally by editing the definition files in node_modules, but this seems to require first compiling, then other steps I'm not familiar with.

Would you mind fixing this yourself the way you suggested? 🙏

EDIT: I implemented the fix, in case you are 100% sure and would like to just merge it

[jscheid]

@Pessimistress thanks for the merge! Any chance for a new release with this fix?