Skip to content

The Map component's declared event listeners are being re-attached on every state update #773

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
shawncrawley opened this issue Mar 28, 2025 · 4 comments · Fixed by #774
Closed

The Map component's declared event listeners are being re-attached on every state update #773

shawncrawley opened this issue Mar 28, 2025 · 4 comments · Fixed by #774

Comments

@shawncrawley
Copy link

shawncrawley commented Mar 28, 2025 •
edited
Loading

I discovered that the Map component's declared event listeners are being re-attached on every state update, which results in the listener firing n times per click, where n is the number of times the parent component's state has been updated since its initial render.

Here's a minimal example, which can be copied and pasted into https://code.esm.sh and reproduced by clicking on the map a few times and then opening the console and checking the log statements. The first click yields one log statement, while the second click results in two (i.e. three total), the third in three (six total), and so forth.

<!DOCTYPE html>
<html>

<head>
  <title>@planet/maps with esm.sh</title>
  <script type="importmap">
    {
      "imports": {
        "htm": "https://esm.sh/htm",
        "react": "https://esm.sh/react@19.0.0",
        "react-dom/": "https://esm.sh/react-dom@19.0.0/",
        "@planet/maps/": "https://esm.sh/@planet/maps/"
      }
    }
  </script>
  <link href="https://esm.sh/ol@10.4.0/ol.css" rel="stylesheet">
  <style>
    html,
    body {
      margin: 0;
    }

    #root {
      position: absolute;
      top: 0;
      height: 100%;
      width: 100%;
    }
  </style>
</head>

<body>
  <div id="root"></div>
  <script type="module">
    import Map from '@planet/maps/Map';
    import View from '@planet/maps/View';
    import ScaleLine from '@planet/maps/control/ScaleLine';
    import TileLayer from '@planet/maps/layer/WebGLTile';
    import OSM from '@planet/maps/source/OSM';
    import htm from 'htm';
    import React from 'react';
    import {createRoot} from 'react-dom/client';

    const html = htm.bind(React.createElement);

    function App() {
      const [count, setCount] = React.useState(0);
      return html`
        <div>Count is ${count}</div>
        <${Map} onClick=${(e)=> console.log("CLICK") || setCount(count+1)} style=${{width: '100%', height: '100%'}}>
          <${View} options=${{center: [0, 0], zoom: 1}} />
          <${TileLayer}>
            <${OSM} />
          </${TileLayer}>
          <${ScaleLine} />
        </${Map}>
      `;
    }

    const root = createRoot(document.getElementById('root'));
    root.render(html`<${App} />`);
  </script>
</body>

</html>
@shawncrawley
Copy link
Author

shawncrawley commented Mar 28, 2025 •
edited
Loading

I found the culprit, but am not quite sure the best resolution.

This line here is being called on every state update, which calls updateInstanceFromProps with oldProps hard-coded to {} and the newProps containing the onClick event function:

maps/lib/Map.js

Line 79 in eb64b23

updateInstanceFromProps(map, MAP, {}, mapProps);

Maybe oldProps could be cached on the map object at the very end of the updateInstanceFromProps function, like so:

map._oldProps = mapProps;

And then the line in Map.js above could be adjusted like so:

updateInstanceFromProps(map, MAP, map._oldProps || {}, mapProps);

I don't know the project well enough to know what ill side-effects that could have, though. Regardless, I'm happy to help however I can!

@tschaub tschaub mentioned this issue Mar 29, 2025
Merged
@tschaub
Copy link
Member

tschaub commented Mar 29, 2025

Thanks for the report, @shawncrawley - and for identifying the root of the issue. I put together a fix in #774, and will cut a release with that before long.

@shawncrawley
Copy link
Author

Thanks for the quick work on that! Again, happy to have helped. Looking forward to the release.

@tschaub
Copy link
Member

tschaub commented Mar 31, 2025

The @planet/maps@11.2.0 release includes the fix.

Thanks again for digging into this and opening the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Maintain a ref to previous map props planetlabs/maps
2 participants
@tschaub @shawncrawley