Wrapper component lifecycle issue

[idolizesc]

The GoogleMaps wrapper component breaks the React lifecycle because it does not re-render the VirtualContainer until after it is done updating everything.

This means trying to get an updated value from a newly-changed Marker, for example, is not possible in any parent component's componentDidUpdate method, because the children of the VirtualContainer are not yet re-rendered.

This is a regression from the previous mixin/context methodology, and leads to very confusing and difficult to debug errors.

Perhaps one solution would be to re-render VirtualContainer on componentWillUpdate instead.

[tomchentw]

I see. Could you show me the reason why you'd like to read properties from a newly-changed Marker? Since if everything is rendered with given props, you already know the value from your data, right?

[idolize]

Well, my use case actually involves reading shape data from a component that renders Circle items. I need to access Google Maps API features, like circle.getBounds(), after some event was triggered.

I suppose I could create a new google.maps.Circle every time from the original input props, but that is annoying and less performant. Also I just feel like it's a reasonable exception for the components to stick to the standard React lifecycle and expose the correct data at that time via the getters.

[tomchentw]

Related to #36

[tomchentw]

Just tried with your use case with react-google-maps/#basics/geolocation, yes, changing callback to componentWillUpdate of GoogleMaps will fix this issue.

However, I'm thinking that the same use case may apply for componentDidMount since we do _render_virtual_container_ in GoogleMaps#componentDidMount as well. As a result, I suspect changing to componentWillUpdate may not be a permanent solution.

[tomchentw]

The VirtualContainer technique prevents the leak of two wrapper divs around the google.maps.Map instance and react components. If we only use one wrapper div, then initialise google.maps.Map instance on it as well as rendering children components inside it. When next update ticks, React will clean out everything created by google.maps.Map instance. Thus only <noscript> tags left inside the <div>

[idolize]

Interesting. React definitely needs some kind of first-class "fragment" element that does not render anything to the DOM. I know they are working on it.

In the mean time, have you looked at react-outlet? It could possibly be of interest here.

Another interesting approach is the way MartyJS creates "container" components: calling Marty.createContainer(SomeComponentToWrap), but also giving you the option to override the method which actually renders the component.

[tomchentw]

facebook/react#2191

[idolize]

Can we just ditch the extra div being created to house the container and instead render the children as either children or siblings of the Google Maps div itself?

This would solve this issue as well as the issue where we can't render any DOM elements as children of the GoogleMaps component.

Also, looking more closely at the code, I believe there is a memory leak in the current implementation, as the containerNode is never actually destroyed on unmount (just set to null), so new container divs will be created each time the component is mounted. (Unless React.unmountComponentAtNode() handles this)

[tomchentw]

@idolize , could you please provide an example of we can't render any DOM elements as children of the GoogleMaps component. Cause I cannot understand why you'd like to do it this way. A simple gist would be enough.

Thanks!

[idolize]

Yeah, my use case is I want to use my own DOM elements to replace some Google Maps controls. The Google Maps API allows you to do this for almost any control (such as the zoom indicator, panning buttons, drawing controls, etc.)

Here is an example of how I could overwrite the drawing controls (which worked in the pre-wrapper component versions of this lib):

import React from 'react';
import {DrawingManager} from 'react-google-maps';

export default React.createClass({
  getInitialState() {
    return { drawingMode: null };
  },
  setDrawingMode(drawingMode) {
    this.setState({ drawingMode });
  },
  render () {
    return (
      <div>
        <DrawingManager
          {...this.props}
          drawingControl={false} {/* Use our own buttons instead of the Google Maps built-in drawing control */}
          drawingMode={this.state.drawingMode}
        />
        <button onClick={this.setDrawingMode.bind(this, google.maps.drawing.OverlayType.CIRCLE)} className={this.state.drawingMode === google.maps.drawing.OverlayType.CIRCLE ? 'active' : ''}>Circle</button>
        <button onClick={this.setDrawingMode.bind(this, google.maps.drawing.OverlayType.POLYGON)} className={this.state.drawingMode === google.maps.drawing.OverlayType.POLYGON ? 'active' : ''}>Polygon</button>
      </div>
    );
  }
});

But this doesn't work with the current implementation because the new drawing controls I try to render don't show up, as they are rendered inside the virtual container div.

The only way to get it to work with the current implementation involves duplicating state (in this case state.drawingMode) inside both the component to render as a child of GoogleMaps as well as inside my other "control" component, and then wiring them together with callbacks 😢.

[tomchentw]

I believe this was fixed in #61. And I certainly believe that I use a elegant way for component lifecycle in the rewrite of #88. Please take a look at it and comments are welcome, thanks!