feat: Support pmtiles.Protocol.add() #93
Conversation
commit: |
Deploying svelte-maplibre-gl with
|
| Latest commit: |
c3b47c0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://447d6a4a.svelte-maplibre-gl.pages.dev |
| Branch Preview URL: | https://pmtiles-protocols-add.svelte-maplibre-gl.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/maplibre/ext/pmtiles/PMTilesProtocol.svelte (1)
38-44: Prevent potential double-inserts.Every time
pmtileschanges, you loop over them again. IfProtocol.add()does not guard against duplicates, you may repeatedly add the same element. Consider deduplicating or removing pmtiles that are no longer present. Otherwise, this approach is valid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/maplibre/ext/pmtiles/PMTilesProtocol.svelte(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: preview-release
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
src/lib/maplibre/ext/pmtiles/PMTilesProtocol.svelte (6)
3-3: Use imports consistently.Great job importing
ProtocolandPMTilesfrompmtiles. Everything here looks correct.
5-10: Consider a default forpmtiles.Currently,
pmtilescan beundefined. If you'd like to always handle an array (even if empty), you might defaultpmtilesto[]. Otherwise, this definition looks clean.
[nirpick]
11-23: Well-documented props.The inline documentation is clear and thoughtful. This ensures that usage is straightforward for future maintainers.
25-28: Initialize with caution.Initializing the
Protocolwith{ metadata, errorOnMissingTile }is correct. Ensure that any subsequent logic (like in$effect) does not inadvertently override uninitialized properties.
30-32: Reactive assignment is concise.This reactive block updates the
metadataproperty whenevermetadatachanges. This is an effective way to maintain synchronization. No issues here.
34-36: Parallel structure forerrorOnMissingTile.Similar to
metadata, this keeps the protocol’serrorOnMissingTilein sync with the prop. This is straightforward and clear.
ciscorn
force-pushed
the
pmtiles-protocols-add
branch
from
e1dae2c to
c3b47c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/maplibre/ext/pmtiles/PMTilesProtocol.svelte(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: preview-release
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
src/lib/maplibre/ext/pmtiles/PMTilesProtocol.svelte (3)
2-3: LGTM! Clean and necessary imports.The imports are well-organized and include all required dependencies for the PMTiles protocol implementation.
5-23: Well-documented props with clear type definitions!The props are well-structured with:
- Comprehensive JSDoc comments explaining each prop's purpose and implications
- Appropriate default values
- Clear TypeScript type definitions
44-44: LGTM! Clean and minimal template.The template correctly binds the necessary props to the MapLibreProtocol component.
Resolves #92