Conversation
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull request overview
This PR fixes a tree-shaking issue by making the extendMapPrototype function public and ensuring it's explicitly called in all examples. This prevents bundlers from removing the prototype extension code in production builds.
Changes:
- Exported
extendMapPrototypefunction with idempotent behavior (returns boolean to indicate if extension occurred) - Added explicit
extendMapPrototype()calls in all 6 example files - Enhanced JSDoc documentation to explain the tree-shaking issue and safe multi-call behavior
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/index.ts | Made extendMapPrototype public, added guard flag for idempotency, changed return type to boolean, and improved documentation |
| examples/zarr/main.ts | Changed from side-effect import to explicit function call |
| examples/zarr-simple/main.ts | Changed from side-effect import to explicit function call |
| examples/react/main.tsx | Changed from side-effect import to explicit function call |
| examples/cog/main.ts | Changed from side-effect import to explicit function call |
| examples/cog-simple/main.ts | Changed from side-effect import to explicit function call |
| examples/basic/main.ts | Changed from side-effect import to explicit function call |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function extendMapPrototype(): boolean { | ||
| if (prototypeExtended) { | ||
| return false; | ||
| } | ||
| prototypeExtended = true; |
There was a problem hiding this comment.
The new exported extendMapPrototype function and its idempotency behavior (using the prototypeExtended flag) lack test coverage. Consider adding tests to verify:
- The function returns
trueon first call andfalseon subsequent calls - The Map prototype is properly extended with all expected methods after calling the function
- Multiple calls don't cause duplicate assignments or errors
This is particularly important since this function is now part of the public API and addresses a critical tree-shaking issue.
The issue was that the examples worked locally during development but failed on GitHub Pages with the error e.setBasemap is not a function. This happened because:
Root Cause: The examples were using a side-effect-only import (import '../../src/index') to extend the Map prototype. In production builds, Rollup/Vite can tree-shake these imports away because no exported values are being used.