Skip to content

Conversation

@kaper-edward
Copy link

카카오맵 react sdk 를 오픈 소스로 구현하고 제공해 주셔서 감사합니다.

이슈 #77 과 관련하여 다음과 같은 코드를 구현해 봤습니다. 관리자님께서 올바른 방향으로 피드백 주시면 개선해 보겠습니다.

  • 지도 초기 로드 시 마커가 표시되지 않는 문제를 해결했습니다.
  • React 18 StrictMode에서 중복 렌더링이 발생하던 문제를 수정했습니다.
  • MarkerClusterer 컴포넌트가 마커의 생명주기를 중앙에서 관리하도록 변경했습니다.
  • API 호출 감소를 위해 마커 연산에 일괄 처리(batch processing)를 추가했습니다.
  • 실제 로컬 데이터를 이용해 클러스터러를 테스트할 수 있는 예제 (basicClusterer.tsx.tsx)를 추가하였습니다.

MarkerClusterer 과 MapMarker 등을 수정 시, 선언적 서술 단계와 명령형 실행 단계를 분리하여, MarkerClusterer가 여러 마커 작업을 단일 API 호출로 묶어 처리할 수 있도록 합니다. 즉, 자식 컴포넌트(MapMarker, CustomOverlayMap)는 Kakao Map 인스턴스를 직접 생성하는 대신, 설명자(descriptor)를 등록하고 실제 인스턴스 생성은 일괄 처리를 위해 MarkerClusterer에게 위임합니다.

테스트 과정에서 큰 도움을 주신 @HaJi490 님께 감사드립니다!

관련 이슈: #77

- 지도 초기 로드 시 마커가 표시되지 않는 문제를 해결했습니다.
- React 18 StrictMode에서 중복 렌더링이 발생하던 문제를 수정했습니다.
- MarkerClusterer 컴포넌트가 마커의 생명주기를 중앙에서 관리하도록 변경했습니다.
- API 호출 감소를 위해 마커 연산에 일괄 처리(batch processing)를 추가했습니다.

MarkerClusterer 과 MapMarker 등을 수정 시, 선언적 서술 단계와 명령형 실행 단계를 분리하여, MarkerClusterer가 여러 마커 작업을 단일 API 호출로 묶어 처리할 수 있도록 합니다. 즉, 자식 컴포넌트(MapMarker, CustomOverlayMap)는 Kakao Map 인스턴스를 직접 생성하는 대신, 설명자(descriptor)를 등록하고 실제 인스턴스 생성은 일괄 처리를 위해 MarkerClusterer에게 위임합니다.

관련 이슈: JaeSeoKim#77"
Comment on lines -46 to -49
/**
* Map에 CustomOverlay를 올릴 때 사용하는 컴포넌트 입니다.
* `onCreate` 이벤트 또는 `ref` 객체를 통해서 `CustomOverlay` 객체에 직접 접근 및 초기 설정 작업을 지정할 수 있습니다.
*/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주석을 제거하게 되는 경우 Docs 생성시 문제가 될 수 있습니다..!
주석과 인터페이스 변경은 없었으면 좋을 것 같습니다.

@JaeSeoKim
Copy link
Owner

MarkerClusterer 과 MapMarker 등을 수정 시, 선언적 서술 단계와 명령형 실행 단계를 분리하여, MarkerClusterer가 여러 마커 작업을 단일 API 호출로 묶어 처리할 수 있도록 합니다. 즉, 자식 컴포넌트(MapMarker, CustomOverlayMap)는 Kakao Map 인스턴스를 직접 생성하는 대신, 설명자(descriptor)를 등록하고 실제 인스턴스 생성은 일괄 처리를 위해 MarkerClusterer에게 위임합니다.

이 부분에 대한 자세한 설명이 필요할 것 같습니다.

자식 컴포넌트(MapMarker, CustomOverlayMap)는 Kakao Map 인스턴스를 직접 생성 <- 요 부분에서 어떤 것이 문제라고 판단하였을까요?

Comment on lines +124 to +126
if (registry) {
return null
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rules Of Hook에 대한 규칙을 어기신것 같은데, 이에 대한 영향은 고려가 되었을까요?

registry.unregister(id)
isMounted.current = false
}
}, [registry, id, allProps, children])
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allProps는 매 렌더링 마다 새롭게 생성되는 객체인데, 이러면 useEffect의 dependency가 되었을 때 어떤 동작이 되기를 기대하신건가요?

const map = useMap("Marker")
const registry = useContext(MarkerClustererContext)

const id = useRef(markerCounter++).current
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 로직을 사용하신 이유가 있을까요?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useId 를 사용하거나, 이에 대한 추상화된 hook를 사용하는 것이 좋아 보입니다.

이건 중요한 포인트는 아니지만, 매 렌더링마다 markerCounter는 + 1 될것 같습니다.

Comment on lines +159 to 247
const flushChanges = () => {
if (!clusterer) {
return
}
}, [map, markerClusterer])

useLayoutEffect(() => {
if (markerClusterer && onCreate) onCreate(markerClusterer)
}, [markerClusterer, onCreate])

useKakaoMapsSetEffect(markerClusterer, "setGridSize", gridSize!)
useKakaoMapsSetEffect(markerClusterer, "setMinClusterSize", minClusterSize!)
useKakaoMapsSetEffect(markerClusterer, "setAverageCenter", averageCenter!)
useKakaoMapsSetEffect(markerClusterer, "setMinLevel", minLevel!)
useKakaoMapsSetEffect(markerClusterer, "setTexts", texts!)
useKakaoMapsSetEffect(markerClusterer, "setCalculator", calculator!)
useKakaoMapsSetEffect(markerClusterer, "setStyles", styles!)

useKakaoEvent(markerClusterer, "clustered", onClustered)
useKakaoEvent(markerClusterer, "clusterclick", onClusterclick)
useKakaoEvent(markerClusterer, "clusterover", onClusterover)
useKakaoEvent(markerClusterer, "clusterout", onClusterout)
useKakaoEvent(markerClusterer, "clusterdblclick", onClusterdblclick)
useKakaoEvent(markerClusterer, "clusterrightclick", onClusterrightclick)

if (!markerClusterer) {
return null

const changes = new Map(changesQueueRef.current)
changesQueueRef.current.clear()

const toAdd: (kakao.maps.Marker | kakao.maps.CustomOverlay)[] = []
const toRemove: (kakao.maps.Marker | kakao.maps.CustomOverlay)[] = []
let newPortals = [...portals]

changes.forEach((descriptor, id) => {
const instance = kakaoInstancesRef.current.get(id)

if (descriptor === null) {
if (instance) {
toRemove.push(instance)
kakaoInstancesRef.current.delete(id)
markerDescriptorsRef.current.delete(id)
const portalIndex = newPortals.findIndex((p) => p.id === id)
if (portalIndex > -1) newPortals.splice(portalIndex, 1)
}
} else if (!instance) {
let newInstance: kakao.maps.Marker | kakao.maps.CustomOverlay | null =
null
const position = new kakao.maps.LatLng(
descriptor.props.position.lat,
descriptor.props.position.lng,
)

if (descriptor.type === "Marker") {
newInstance = new kakao.maps.Marker({ ...descriptor.props, position })
} else if (descriptor.type === "CustomOverlayMap") {
const container = document.createElement("div")
newInstance = new kakao.maps.CustomOverlay({
...descriptor.props,
position,
content: container,
})
if (descriptor.children) {
newPortals.push({
id,
container,
children: descriptor.children,
})
}
}

if (newInstance) {
kakaoInstancesRef.current.set(id, newInstance)
markerDescriptorsRef.current.set(id, descriptor)
toAdd.push(newInstance)
}
} else {
const newPosition = new kakao.maps.LatLng(
descriptor.props.position.lat,
descriptor.props.position.lng,
)
instance.setPosition(newPosition)
instance.setZIndex(descriptor.props.zIndex || 0)

if (
instance instanceof kakao.maps.Marker &&
"image" in descriptor.props
) {
const imageProps = descriptor.props.image
if (imageProps) {
const markerImage = new kakao.maps.MarkerImage(
imageProps.src,
new kakao.maps.Size(
imageProps.size.width,
imageProps.size.height,
),
imageProps.options,
)
instance.setImage(markerImage)
}
}
markerDescriptorsRef.current.set(id, descriptor)
}
})

if (toRemove.length > 0) clusterer.removeMarkers(toRemove, true)
if (toAdd.length > 0) clusterer.addMarkers(toAdd, true)

setPortals(newPortals)
clusterer.redraw()
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

각 코드에 대한 역할과 책임이 명확하게 분리되어야 할 것 같습니다.

현재는 조건 분기가 많아 어떠한 사이드 이펙트가 발생하는지 명확하게 알기 어렵습니다.

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.

2 participants