[Feat] Allow <Pin> glyphs to be passed as children

[benschlegel]

Target Use Case

Currently, the only way pass a glyph thats an HTMLElement to a <Pin> element (as far as i can tell) is to create the element inline in a react component and pass that to the pin like in the following example:

export default function MyComponent() {
  const glyphIcon = document.createElement('img');
  glyphIcon.src = "/custom-icon.svg";

  return (
    <>
      <AdvancedMarker>
        <Pin glyph={glyphIcon}/>
      </AdvancedMarker>
    </>
  )
}

This also causes unexpected issues when trying to render multiple markers using Array.prototype.map(), where defining the element just once in the component will only render it out on the last AdvancedMarker, since the native DOM and reacts virtual DOM have some conflicts when using document.createElement(). The workaround is to move the document.createElement() part down into map, but still not a clean way to interact with react.

Instead, I think it would be much nicer if we could pass a glyph via react children, similar to how <MapControl> currently functions, which renders the children to appropriate HTMLElements to be used with the google maps api. As this is a react wrapper, removing the need to manually create DOM elements directly would be inline with what this library is trying to achieve.

This would increase code maintainability, decrease weird "non-react-like" code and decrease bug encounters when trying to interface with native DOM Elements and reacts virtual DOM when using this library (as well as just making the API easier to use).

Proposal

Ideally, when trying to pass a glyph thats an HTMLElement to a <Pin>, we could do something like this:

<AdvancedMarker>
  <Pin background={...}>
    <img src="/icon.svg" />
  </Pin>
</AdvancedMarker>

Where <Pin> interprets children that get passed as glyphs of type Element (according to types here) and gets rendered out appropriately.

[benschlegel]

If this is something this library wants to support, I could take a look at implementing this myself (based on the way MapControl is implemented) and creating an PR, if the manpower/priority for this is not available.

[usefulthink]

You're probably right. I never thought that would actually be useful, but you're proving me wrong :)

If you find the time to implement this, that would be awesome. I can assist you when needed.

Here's a couple of thoughts on the topic:

• when a child is present, we use a portal to render it into pinElement.glyph.
• The portal needs a target element that exists when the portal is created, which means that there has to be a wrapper around the child in pinElement.glyph. So in your example, it would be like <div><img src="/icon.svg" /></div> instead of just <img src="/icon.svg" />. Does this cause any problems? Or can the rendered component itself be retrieved and moved onto pinElement.glyph?
• what happens if both children and a glyph prop are specified?
• what should happen if more than one child is found?

[benschlegel]

If its alright with you, I'll just start a PR draft with basic functionality working where we can discuss implementation and other details over there so we have actual code to reference.

Some thoughts on what you mentioned:

• could we just do this similar to how the portal in map-control.tsx is handled, like in this line?https://github.com/visgl/react-google-maps/blob/cb40e205cf19f95e6a58ab738cda8782d0887908/src/components/map-control.tsx#L50C77-L50C77

• I think the children should take priority, as they need to be explicitly passed and could be inferred as a glyph of type Element, maybe there's some typescript magic we can do that makes them exclusive as well (either inline glyph of type string | URL | undefined | ... or children but no glyph prop on Pin)

• we should probably throw an error of some sorts (unless its actually possible to render multiple children down into one element with some weird <></> logic? But only supporting one child out of the box seems like it makes more sense to me). React outlines a way to get the amount of children in the official docs here, so that part should be possible.

[usefulthink]

That would be great. For development, I would recommend you clone the repository and update the example in /examples/markers-and-infowindows to include a test for the new feature. Then you can run npm run start-local in that example directory and work on the pin.tsx component to implement the new feature.

could we just do this similar to how the portal in map-control.tsx is handled [...]

absolutely, but in this case there needs to be an extra element. I don't see a problem with that right now, but we'll probably have to see.

As for the "both children and glyph specified" case, I think we should at least log an error in that case (you can use the logErrorOnce function from ./src/libraries/errors.ts for that).

[...] maybe there's some typescript magic we can do [...]

That's an interesting Idea, maybe something like

export type PinProps = 
  | PropsWithChildren<Omit<google.maps.marker.PinElementOptions, "glyph">> 
  | google.maps.marker.PinElementOptions

will already do the trick?!

[benschlegel]

Thanks, I'll take a look and see if I can get it working!

As for the type thing, I looked into it for a bit, but unfortunately, it seems to be a limitation of typescript/tsx types, where Components don't error for wrong types even if the prop type is "correct" on paper. We can still use your type for DX, but even with using that type, doing something like:

<Pin glyph={"a"}>
	<div/>
</Pin>

does not show a type error unfortunately. I think we should still use the type for better maintainability, but there does not seem to be a way for TS to show users problems with using the component before runtime.

[benschlegel]

Quick question, how can i rebuild the main source library/use the updated <Pin> component in the example?
I'm getting the example to run and make changes to it like you described, but the example does not seem to use changes i make to pin.tsx in the main src directory.

(maybe my changes also just did not work, but after suspecting my changes were not applied, I put a console.error/log/logErrorOnce at the start of pin.tsx which are also not showing up in the console when running the example for me)

I tried npm build, build:examples, prepack, start and npm build in the example dir, any help would be appreciated.

[usefulthink]

You have to use the npm run start-local script. That one uses a special vite.config.js that uses the library from the ./src directory for @vis.gl/react-google-maps instead of the one installed from npm.

[benschlegel]

thank you!! I must have mixed up your command with the one i saw in the example readme, but using npm run start-local works as expected