Pre-Refactoring for React +16.3

[maulberto3]

Hi Michele, how are you? I hope great. I plan to do gradual PRs until this repo reaches React +16.3. This PR lays foundations for that. New PRs should follow refactoring for +16.3, hopefully using new lifecycle methods and ditching deprecated ones. Notice that everything works using the source components. However, can't say why some tests fail (npm run test), or maybe due to the (npm run) prepublish command which seems not to babelify code as expected, or I am missing something. Let me know your thoughts. Best, Mauricio.

[maulberto3]

Will be addressing the failed tests as Travis gives some insights on that. Thanks.

[maulberto3]

Hi @MicheleBertoli, any insights on how to address the said Travis errors?

[MicheleBertoli]

Thank you very much @maulberto3 for working on this, it's super helpful.

So, the tests are failing because the Entity component needs to have an entity property while in your refactoring you put the entity into the state:

state = {
  entity: null,
};

In general I wouldn't change the demo, as I like to keep it simple, and it seems there are some problems with the formatting of the code (tabs vs spaces) - I guess we need prettier.

For example:

[maulberto3]

Hi @MicheleBertoli ok, let do some work according to your suggestions. I'll get back to you soon. Thank you.

[maulberto3]

Hi @MicheleBertoli. All tests passed, as well as (default rules of) prettier applied to source code. I hope it meets your requirements. If not, please let me know, happy to listen.

If it's ok with you, by the end of the entire refactor, I can leave everything as it was as right now the demo is modified showing more components than originally (I just want to make sure everything is alive and kicking in the browser :) ).

If you agree, I can move on to refactor towards specifics React 16.3, for example componentWillReceiveProps() methods to static getDerivedStateFromProps() and so on.

Best, Mauricio.

[MicheleBertoli]

Thank you very much @maulberto3 for updating the diff.

I love Prettier, but I think we should add it in a separate PR otherwise it's hard to tell which changes are related to the refactoring and which ones are related to the new formatting.

I also see a couple of problems that should be addressed before merging:

• it's fine to remove the Listener mixin but I wouldn't copy its functions into the components. We should turn the mixin into a higher-order component.
• the map property of the GMaps component is now stored into the state. This is not optimal because in the state should go only data that is used in the render method since it causes a re-render.

I hope this makes sense for you.

[maulberto3]

Hi @MicheleBertoli, been trying to do the said HOC but still no success. Lots of errors at test time. Any hints would be appreciated. Sorry for the unnecessary delay. Code attached:

class Gmaps extends React.Component {
  map = null;

  state = {
    isMapCreated: false
  };

  componentDidMount() {
    this.setState({
      callbackIndex: GoogleMaps.load(this.props.params, this.mapsCallback)
    });
  }

  componentWillUnmount() {
    GoogleMaps.removeCallback(this.state.callbackIndex);
    this.removeListeners();
  }

  componentWillReceiveProps(nextProps) {
    if (this.map && !compareProps(this.props, nextProps)) {
      this.map.setOptions({
        ...nextProps,
        center: new google.maps.LatLng(nextProps.lat, nextProps.lng)
      });
    }
  }

  getMap() {
    return this.map;
  }

  mapsCallback = () => {
    this.createMap();
    this.addListeners(this.map, MapEvents);
  };

  createMap = () => {
    const node = ReactDOM.findDOMNode(this);
    this.map = new google.maps.Map(node, {
      ...this.props,
      center: new google.maps.LatLng(this.props.lat, this.props.lng)
    });
    this.setState({ 
      isMapCreated: true 
    });
    if (this.props.onMapCreated) {
      this.props.onMapCreated(this.map);
    }
  };

  getChildren() {
    return React.Children.map(this.props.children, child => {
      if (!React.isValidElement(child)) {
        return child;
      }
      return React.cloneElement(child, {
        ref: child.ref,
        map: this.map
      });
    });
  }

  render() {
    const style = objectAssign({
        width: this.props.width,
        height: this.props.height
      },
      this.props.style);
    return (
      <div style={style} className={this.props.className}>
        {this.props.loadingMessage || 'Loading...'}
        {this.state.isMapCreated ? this.getChildren() : null}
      </div>
    );
  }
};


function WithListeners (WrappedComponent) { // input is Gmaps component above
  return class Gmaps2 extends React.Component {
    addListeners(entity, events) {
      for (let prop in this.props) {
        if (this.props.hasOwnProperty(prop) && events[prop]) {
          const addListener = google.maps.event.addListener;
          const listener = addListener(entity, events[prop], this.props[prop]);
          if (!this.listeners) {
            this.listeners = [];
          }
          this.listeners.push(listener);
        }
      }
    }

    removeListeners() {
      if (window.google && this.listeners) {
        this.listeners.forEach(listener => {
          google.maps.event.removeListener(listener);
        });
      }
    }

    render(){
      return(
        <WrappedComponent {...this.props} />
      );
    }
    
  };
};

const GmapsWithListeners = WithListeners(Gmaps);

export default GmapsWithListeners;

[MicheleBertoli]

Hello @maulberto3, it seems you need to pass down addListeners and removeListeners as props.

I implemented the following higher-order component:

const withListeners = Component =>
  class extends React.Component {
    addListeners = (entity, events) => {
      for (let prop in this.props) {
        if (this.props.hasOwnProperty(prop) && events[prop]) {
          const addListener = google.maps.event.addListener;
          const listener = addListener(entity, events[prop], this.props[prop]);
          if (!this.listeners) {
            this.listeners = [];
          }
          this.listeners.push(listener);
        }
      }
    }

    removeListeners = () => {
      if (window.google && this.listeners) {
        this.listeners.forEach((listener) => {
          google.maps.event.removeListener(listener);
        });
      }
    }

    render() {
      return (
        <Component
          {...this.props}
          addListeners={this.addListeners}
          removeListeners={this.removeListeners}
        />
      );
    }
  };

and changed the references to the functions in entity and gmaps, like:

     componentDidMount() {
       const options = this.getOptions(this.props);
       this.entity = new google.maps[name](options);
-      this.addListeners(this.entity, events);
+      this.props.addListeners(this.entity, events);
     },

and it works.

I hope this helps.

[maulberto3]

:O Hi, will try to commit soon using your implementation. Thank you.

[MicheleBertoli]

Thank you very much for your contribution, @maulberto3

I'm closing this due to inactivity and v2 coming soon (#130)