Skip to content

fix: AnimatedRegion.addListener() property setting#3131

Merged
christopherdro merged 1 commit intoreact-native-maps:masterfrom
cruzach:patch-1
Oct 17, 2019
Merged

fix: AnimatedRegion.addListener() property setting#3131
christopherdro merged 1 commit intoreact-native-maps:masterfrom
cruzach:patch-1

Conversation

@cruzach
Copy link
Copy Markdown
Contributor

@cruzach cruzach commented Oct 14, 2019

Does any other open PR do the same thing?

I searched and didn't find any open PRs relating to AnimatedRegion

What issue is this PR fixing?

expo/expo#4574
facebook/react-native#25224

How did you test this PR:

Locally in an Expo SDK 35 project. Step-by-step testing: initialize a project with the following code

import React, { Component } from "react";
import { View, Text } from "react-native";
import MapView, {
  AnimatedRegion,
  Animated,
  Marker,
  MarkerAnimated
} from "react-native-maps";

export default class Test extends Component {
  constructor() {
    super();
    this.state = {
      coordinate: new AnimatedRegion({
        latitude: -33.07204,
        latitudeDelta: 0.01,
        longitude: -68.98632,
        longitudeDelta: 0.005622188905547227
      })
    };

    // without these lines works well
  }

  async componentDidMount() {
    const listener = this.state.coordinate.addListener(value => {
      console.log(value);
    });

    const duration = 5000;

    const coord = {
      latitude: -33.0735,
      latitudeDelta: 0.01,
      longitude: -68.98632,
      longitudeDelta: 0.005622188905547227
    };

    this.state.coordinate
      .timing({
        ...coord,
        duration
      })
      .start();
  }

  render() {
    const { coordinate } = this.state;

    return (
      <View style={{ flex: 1 }}>
        <Text>test...</Text>
        <MapView
          style={{ flex: 1 }}
          initialRegion={{
            latitude: -33.07204,
            latitudeDelta: 0.01,
            longitude: -68.98632,
            longitudeDelta: 0.005622188905547227
          }}
        >
          <MarkerAnimated
            ref={ref => {
              this.marker = ref;
            }}
            title={"tirulo"}
            description={"description"}
            coordinate={coordinate}
          />
        </MapView>
      </View>
    );
  }
}

and then run expo start. It will fail. Make changes in this PR, and it will work like a charm.

@christopherdro christopherdro merged commit dd2a10d into react-native-maps:master Oct 17, 2019
@cruzach cruzach deleted the patch-1 branch October 17, 2019 21:47
@cgsdev0
Copy link
Copy Markdown
Contributor

cgsdev0 commented Oct 18, 2019

I've been testing this out today; I don't think this actually really solves the problem.

As far as I can tell, this is not the expected usage of Object.defineProperty https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty

In reality, the id each of the 4 addListener calls returns is lost; this breaks removeListener. I tried updating the usage of defineProperty to the following, but it broke the AnimatedRegion functionality entirely:

  addListener(callback) {
      var id = String(_uniqueId++);
      var jointCallback = (/*{value}*/) => {
        callback(this.__getValue());
      };
      object.defineProperty(this._listeners, id, {
        enumerable: true,
        configurable: true
        value: {
          latitude: this.latitude.addListener(jointCallback),
          longitude: this.longitude.addListener(jointCallback),
          latitudeDelta: this.latitudeDelta.addListener(jointCallback),
          longitudeDelta: this.longitudeDelta.addListener(jointCallback),
        }
      };
      return id;
    }
 

It seems like react native doesn't actually require us to store the ids in the _listeners object. The only other instance of this pattern I could find is AnimatedValueXY. I've been playing around with a fix that stores the listener ids locally in an object and it seems to work as intended. I'll open a PR shortly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants