milespratt / bingmaps-react

An easy to use Bing Maps React component.
https://bingmaps-react.netlify.app
MIT License
21 stars 19 forks source link

Binding pushpins does not work #29

Closed cmhernandezdel closed 3 years ago

cmhernandezdel commented 3 years ago

If you bind the pushpins property to a state variable in the parent, like this...

const [pushpins, setPushpins] = useState([]);

...

<BingMap pushPins={pushpins} ... />

And then update the pushpins variable via an API for example, the map does not load and you get errors like this: Uncaught TypeError: Microsoft.Maps.NetworkCallbacks.f_logCallbackRequest is not a function

Looks like the calls are as follows: useEffect, initMap, appendBingMapsScript, useEffect, initMap and here it calls again appendBingMapsScript, which it shouldn't. Then it calls makeMap twice.

cmhernandezdel commented 3 years ago

I will add more information:

Seems like a race condition in the initMap function. If the API request that fetches the points (in the parent) finishes before the bing maps script is executed, then window.Microsoft is not defined and appendBingMapsScript is called again, causing the issue. Adding a delay in the API response solved it for me, so it looks like the problem lays there.

milespratt commented 3 years ago

I will add more information:

Seems like a race condition in the initMap function. If the API request that fetches the points (in the parent) finishes before the bing maps script is executed, then window.Microsoft is not defined and appendBingMapsScript is called again, causing the issue. Adding a delay in the API response solved it for me, so it looks like the problem lays there.

Would you mind sharing the code you're using to update your pushpins? I'm able to update the pushpins via API without issue.

cmhernandezdel commented 3 years ago

Sorry, could not answer before. The code should look something like this (in page component):

const [pushpin, setPushpin] = useState({'latitude': null, 'longitude': null});

useEffect( () => {
    const fetchData = async () => {
        try {
            const response = await axios.get(url);
            setPushpin( { 'latitude': response.data.latitude, 'longitude': response.data.longitude } );
        } catch (error) {
            ...
        }     
    };
    fetchData();
});

... // other code

<BingMap latitude={pushpin.latitude} longitude={pushpin.longitude} />

The problem looks like the API call is faster than loading the script (appendBingMapsScript function), so the props change and then map rerenders and since script is not yet loaded (window.Microsoft is not defined yet). appendBingMapsScript is called again and causes the error.

milespratt commented 3 years ago

Sorry, could not answer before. The code should look something like this (in page component):

const [pushpin, setPushpin] = useState({'latitude': null, 'longitude': null});

useEffect( () => {
    const fetchData = async () => {
        try {
            const response = await axios.get(url);
            setPushpin( { 'latitude': response.data.latitude, 'longitude': response.data.longitude } );
        } catch (error) {
            ...
        }     
    };
    fetchData();
});

... // other code

<BingMap latitude={pushpin.latitude} longitude={pushpin.longitude} />

The problem looks like the API call is faster than loading the script (appendBingMapsScript function), so the props change and then map rerenders and since script is not yet loaded (window.Microsoft is not defined yet). appendBingMapsScript is called again and causes the error.

Thanks for the additional info. I am able to replicate the error you're getting but not in the specific scenario you're calling out. Could you please provide your actual code for me to take a look at? My component does not support a single push pin like in your example, nor does it support a latitude and longitude prop. A specific code example would help a lot.

Thanks!

milespratt commented 3 years ago

Sorry, could not answer before. The code should look something like this (in page component):

const [pushpin, setPushpin] = useState({'latitude': null, 'longitude': null});

useEffect( () => {
    const fetchData = async () => {
        try {
            const response = await axios.get(url);
            setPushpin( { 'latitude': response.data.latitude, 'longitude': response.data.longitude } );
        } catch (error) {
            ...
        }     
    };
    fetchData();
});

... // other code

<BingMap latitude={pushpin.latitude} longitude={pushpin.longitude} />

The problem looks like the API call is faster than loading the script (appendBingMapsScript function), so the props change and then map rerenders and since script is not yet loaded (window.Microsoft is not defined yet). appendBingMapsScript is called again and causes the error.

I just released an update that includes a new prop that should help with the async nature of the map loading. Please update to 1.1.0 to try it out.

The new onMapReady prop takes a function which will run when the map has finished loading. I recommend you use this to trigger any API calls you're making at the same time the map is loading to ensure it has already loaded before updating props.

Here is an example:

function App() {

  // state for pushpins
  const [pushPins, setPushPins] = useState([]);

  // state for map readiness
  const [mapReady, setMapReady] = useState(false);

  // function to get and set pushpins
  function getPushPins() {
    ... // API call for pushpins
    setPushPins(API result)
  }

  useEffect(() => {
    // get pushpins after map is ready
    if (mapReady) {
      getPushPins();
    }
  }, [mapReady]);

  return (
    <div className="map__container">
      <BingMapsReact
        onMapReady={() => setMapReady(true)} // set map ready function
        pushPins={pushPins}
      />
    </div>
  );
}
cmhernandezdel commented 3 years ago

Hi, I will try it probably on Friday or Saturday and come back to you.

Thank you for your time!