uiwjs / react-amap

基于 React 封装的高德地图组件,帮助你轻松的接入地图到 React 项目中。
https://uiwjs.github.io/react-amap
MIT License
428 stars 70 forks source link

polygon,react组件在初始化的时候被销毁,会导致高德地图polygon无法被销毁 #341

Closed starInEcust closed 12 months ago

starInEcust commented 1 year ago

又发现了一个惊天大bug 如果在 初始化阶段,外层组件同时把它销毁了, 那这个多边形在高德地图的层面永远不会被销毁

  useEffect(() => {
    if (!AMap || !map) return;
    if (!polygon) {
      console.log('new');
      let instance: AMap.Polygon = new AMap.Polygon({ ...other });
      map.add(instance);
      setPolygon(instance); // setState之后本来应该再次触发useEffect,并且更新return里的函数
    }
// 但是外层组件直接给它销毁了,它永远都不会remove
    return () => {
      console.log('return', polygon);
      if (polygon) {
        try {
          console.log('remove');
          map && map.remove(polygon);
        } catch (e) {}
        setPolygon(undefined);
      }
    };
  }, [map, polygon]);

我还没有查看除了这个polygon有没有别的组件也是这样的。

starInEcust commented 1 year ago

我写了个demo来模拟这个问题,这个demo里Child这个count永远也等不到3了

function App() {
  const [count, setCount] = useState(0);
  console.log("app render", count);
  useEffect(() => {
    console.log("app mount and set count 1");
    setCount(1);
  }, []);

  return <>{count === 0 && <Child />}</>;
}

const Child = () => {
  const [count, setCount] = useState(0);
  console.log("child render count", count);
  useEffect(() => {
    console.log("child useEffect");
    if (count === 0) {
      console.log("child set 3");
      setCount(3);
    }

    return () => {
      console.log("child unmount", count);
    };
  }, [count]);
  return null;
};
image

我发现原来的那个版本里比如5.0.6是这样写的,这个我测试下来是没问题的,要不要改回去,我来提个pr

  useEffect(() => {
    if (!AMap || !map) return;
    if (!polygon) {
      let instance: AMap.Polygon = new AMap.Polygon({ ...other });
      map.add(instance);
      setPolygon(instance);
      return () => {
        if (instance) {
          try {
            map && map.remove(instance);
          } catch (e) {}
          // if (AMap.v) {
          //   map && map.remove(instance);
          // } else {
          //   // 暂不使用这个 API,这个不兼容 v1.4.xx,改用 map.remove API
          //   map && map.removeLayer(instance);
          // }
        }
        setPolygon(undefined);
      };
    }
  }, [map]);
jaywcjlove commented 1 year ago

@starInEcust 可以

jaywcjlove commented 1 year ago

@starInEcust 期待您的 PR

starInEcust commented 1 year ago

不行啊- - 我看貌似好像所有模块都是这么写的,都有这么个毛病,总不能就改一个吧- - 还有我怎么folk过来起不来呀,跑别的啥命令嘛

image
jaywcjlove commented 1 year ago

@starInEcust 需要按照下面顺序跑命令

  1. 安装依赖
npm install
  1. 编译所有包,包没有编译会造成包的文件不存在
npm run build 

https://github.com/uiwjs/react-amap/blob/aa66c83cba5a2630e417f96f4c158334fa8128bd/.github/workflows/ci.yml#L21-L23

starInEcust commented 1 year ago

所以是要都改了嘛~ 这不是开历史倒车,有点不敢呀。 当时为啥要改成现在这样,是顺手好看还是有别的bug~

jaywcjlove commented 1 year ago

@starInEcust 维护的东西太多,当时什么想法现在也忘记了,现在的这个写法永远都不会 remove 是正常的,因为卸载的时候 polygon 已经变成了 undefined ,不存在。

  useEffect(() => {
    if (!AMap || !map) return;
    if (!polygon) {
      console.log('new');
      let instance: AMap.Polygon = new AMap.Polygon({ ...other });
      map.add(instance);
      setPolygon(instance); // setState之后本来应该再次触发useEffect,并且更新return里的函数
    }
  // 但是外层组件直接给它销毁了,它永远都不会remove
    return () => {
      console.log('return', polygon);
      if (polygon) {
        try {
          console.log('remove');
          map && map.remove(polygon);
        } catch (e) {}
        setPolygon(undefined);
      }
    };
  }, [map, polygon]);

老的写法,可能是正确的

if (!polygon) {
  let instance: AMap.Polygon = new AMap.Polygon({ ...other });
  map.add(instance);
  setPolygon(instance);
  return () => {
    if (instance) {
      try {
        map && map.remove(instance);
      } catch (e) {}
      // if (AMap.v) {
      //   map && map.remove(instance);
      // } else {
      //   // 暂不使用这个 API,这个不兼容 v1.4.xx,改用 map.remove API
      //   map && map.removeLayer(instance);
      // }
    }
    setPolygon(undefined);
  };
}