chore(deps): bump maplibre-gl to 6.0.0 - #1510
Conversation
Clears blocker 2 of #1489 and carries the GeoLibre-side migration. Blocker 1's two external packages are handled by a temporary shim rather than being waited on, so the whole tree builds, tests, and runs on v6 today. v6 is ESM-only with no default export and no CJS build. **Blocker 2 — the frontend suite.** The plugin packages' `exports` maps are correct; the problem was our own module scope. Every workspace package is `"type": "module"`, but the root package.json is not, so tsx compiled all 256 test files to CJS, which selects the `require` condition and loads `.cjs` entries that `require("maplibre-gl")`. `tests/package.json` puts them in ESM scope, and the 29 ERR_PACKAGE_PATH_NOT_EXPORTED failures go to zero. This is a swap, not an addition — v5 ships no ESM build, so it cannot land without the bump. **Blocker 1 — the two external packages.** `@esri/maplibre-arcgis` and `@geoman-io/maplibre-geoman-free` still default-import maplibre-gl in their published ESM. Both are rewritten to namespace imports at load: a Vite plugin for the app build, a Node loader hook for `node --test` (Vite cannot reach the test runner), and an `optimizeDeps.exclude` entry because the dependency optimizer runs outside the plugin pipeline. The two lists are kept in step by `tests/maplibre-shim-parity.test.ts`, and the Vite plugin errors if a package stops matching, so a package that ships a fix cannot be silently shimmed forever. **The worker.** v6 ships its worker as a separate file located at runtime with `new URL("./maplibre-gl-worker.mjs", import.meta.url)` — a computed string no bundler can see. The asset was never emitted, and the URL resolved next to the hashed app chunk, where the SPA fallback answered with index.html, so the request hung instead of 404ing. Tile parsing was silently degraded: the same 25s window pulled 39 tiles before the fix and 68 after. Fixed by emitting the worker through Vite and pointing `setWorkerUrl` at it. Also: - 30 files migrated to `import * as maplibregl from "maplibre-gl"`. - Root `overrides` pin, because `@maplibre/maplibre-gl-directions` still peers `^5.0.0` and would otherwise resolve a second hoisted copy, making `Map` a different nominal type across package boundaries. - `packages/map/src/dynamic-style-property.ts` holds the casts for property names computed at runtime, now that set/getPaint/LayoutProperty are generic over `keyof AllPaintProperties`. - Geoman's `gm:*` events need a cast: `Map#on`/`off`'s catch-all overload narrowed from `type: string` to `keyof MapEventType`. - `GeoAgentSyncableTools`' map slice uses method syntax so a real `Map` still satisfies it under `strictFunctionTypes`. - GeolocateControl is constructed through a factory so tests can substitute a fake — the sealed v6 namespace rejects assignment (same root cause as #1509). - jsPDF is imported by name: its `node` export condition resolves a CJS bundle whose default is the module object, which only surfaced once the tests moved to ESM scope. Verified: test:frontend 4236 passed / 0 failed, coverage 86.93/84.15/71.13, tsc -b + vite build clean, lint 0 errors, test:worker clean. e2e 19/23 locally with all four failures reproducing on this machine independent of the bump (each passes in isolation). Dev server, production build, Geoman's toolbar and the Esri package all verified in a browser.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Cloudflare PR preview
|
🔍 GitHub Pages PR preview
|
Two import conflicts, both from main editing the same maplibre-gl import lines this branch is converting for v6 (which has no default export): - packages/plugins/src/plugins/maplibre-annotations.ts: main promoted the type-only default import to a value import because it added runtime use of `maplibregl.LngLat`/`maplibregl.Marker`. Resolved to a value namespace import (`import * as maplibregl`) so both the runtime use and the v6 no-default-export rule hold. - apps/geolibre-desktop/src/components/layout/MapContextMenu.tsx: kept main's added lucide-react icons and `useMemo`, with this branch's `import type * as maplibregl` (still type-only here). Verified no default import of maplibre-gl survives anywhere in first-party code, then ran the gate: build, 4612 frontend tests, worker typecheck.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| // Matches a default import of maplibre-gl in minified or unminified ESM: | ||
| // import Zt from"maplibre-gl" import e from "maplibre-gl" | ||
| const DEFAULT_IMPORT = /\bimport\s+([A-Za-z_$][\w$]*)\s+from\s*(["'])maplibre-gl\2/g; |
There was a problem hiding this comment.
Quality/robustness (low-medium confidence): DEFAULT_IMPORT only matches a bare default import (import X from "maplibre-gl"). If either shimmed package's published bundle ever emits a combined default+named import, e.g. import X, { Y } from "maplibre-gl", this regex won't match (no from immediately after the identifier), so the rewrite — and the buildEnd staleness check — would silently miss it, and the build would fail downstream with the raw [MISSING_EXPORT] "default" error instead of the intended clear this.error() message.
tests/maplibre-shim-parity.test.ts only asserts against the two current known bundle shapes, so this would only surface on a future dependency bump that changes the import form. Not a problem today, but worth a short comment noting the regex's coverage is intentionally narrow (or extending it), so it doesn't quietly bit-rot alongside the "list only ever shrinks" invariant this file documents.
|
That posted successfully. Now let me finalize with the summary — the null-byte finding couldn't be posted inline (GitHub rejects comments on binary-diffed files), so it needs to go in the summary instead. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
| import { setWorkerUrl } from "maplibre-gl"; | ||
| // Vite bundles the worker (it imports `./maplibre-gl-shared.mjs`) and hands back | ||
| // the emitted asset URL. | ||
| import maplibreWorkerUrl from "maplibre-gl/dist/maplibre-gl-worker.mjs?worker&url"; |
There was a problem hiding this comment.
Minor/low-confidence nit: this hardcodes maplibre-gl/dist/maplibre-gl-worker.mjs as the worker entry path. The PR wisely added tests/maplibre-shim-parity.test.ts to guard the other MapLibre-v6 workaround (the import shim) against silently rotting when the dependency updates, but there's no equivalent guard here — if a future maplibre-gl patch renames/moves this dist file, the failure mode is exactly the one this PR just fixed (a silently-hanging worker request), and nothing in CI would catch it. Worth considering a cheap existence check (e.g. an e2e/smoke assertion that the worker actually loads, or at least a comment pointing at what to re-verify on maplibre-gl bumps) so a future Dependabot bump doesn't quietly reintroduce this bug.
|
Both findings are posted. Now for the final summary comment. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
|
|
||
| // Matches a default import of maplibre-gl in minified or unminified ESM: | ||
| // import Zt from"maplibre-gl" import e from "maplibre-gl" | ||
| const DEFAULT_IMPORT = /\bimport\s+([A-Za-z_$][\w$]*)\s+from\s*(["'])maplibre-gl\2/g; |
There was a problem hiding this comment.
This regex only matches a bare default import (import X from "maplibre-gl"). If either shimmed package's bundle ever contains a combined default+named import — import X, { Y } from "maplibre-gl" — this won't rewrite it, and the module still has an unsatisfiable default import going into Rollup.
That in itself would likely fail loudly ([MISSING_EXPORT] "default" is not exported...), so it probably wouldn't ship silently. But the buildEnd staleness guard (lines 56-65) checks only whether the package had at least one rewrite (rewritten.add(pkg)), not whether every matching file in that package was successfully transformed. So if one file in a shimmed package uses the plain form (caught) and another uses the combined form (missed), the guard stays green while the second file still breaks the build — defeating part of the point of the guard, which is to make gaps visible rather than have them surface as an opaque bundler error.
Low confidence this pattern actually appears in either package's current bundle (no way to check node_modules in this environment), but worth a quick check, and maybe worth extending the regex to also match import X, { / import X, * as forms for robustness.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall the migration is careful and the reasoning for each workaround (worker URL, shim, generic paint/layout property types, sealed ESM namespace, |
The KML super-overlay protocol registration (merged from main) reached for `maplibre.default.addProtocol` as a fallback for maplibre-gl's CJS build. v6 is ESM-only with no default export and no CJS build, so that property no longer exists and `tsc -b` failed the build in every job that compiles the app (CI build, E2E, website, PR preview, container image). Destructure the named export directly, matching the shape already used in maplibre-reverse-geocode.ts.
| assert.deepEqual(shimmedPackagesInVitePlugin(), [...SHIMMED_PACKAGES]); | ||
| }); | ||
|
|
||
| it("rewrites the default import forms both packages actually publish", async () => { |
There was a problem hiding this comment.
Quality (medium confidence): the two shims each hard-code their own copy of DEFAULT_IMPORT (apps/geolibre-desktop/vite-plugins/maplibre-default-import-shim.ts and tests/hooks/maplibre-default-import-shim.mjs), and this suite only checks that the two SHIMMED_PACKAGES lists agree — the rewrite regexes' behavior is only exercised against the .mjs copy (via rewriteDefaultImports, this test). If the Vite plugin's regex is ever tweaked (e.g. to handle a new default-import shape one of the shimmed packages starts publishing) and the .mjs copy isn't updated to match, this suite stays green even though the two shims now diverge — the dev-server/test-runner path would silently rewrite differently than the production build path. Consider asserting the two regex sources are identical (or extracting DEFAULT_IMPORT/rewriteDefaultImports into a single module both files import) so a future edit to one is forced to update the other.
Code reviewBugs: None found. Traced the core mechanics carefully — Security: None found. The regex-based source rewriting only touches two explicitly named, trusted Performance: None found. Both the Vite Quality:
CLAUDE.md: No violations found. The v5→v6 bump correctly threads through the three build surfaces (web, Tauri desktop, embed all share one Vite entry/config), no new external tile/style hosts were added (so no CSP allowlist update needed), and no new user-facing strings were introduced requiring i18n catalog updates. |
Conflicts: - vite.config.ts: keep both new imports (the v6 default-import shim from this branch, the guarded proxy helper from main). - maplibre-geo-editor.ts: keep this branch's ThirdPartyEventTarget slice for Geoman's `gm:*` events on top of main's move of GEO_EDITOR_PLUGIN_ID into ../plugin-ids. Also adapts main's new map-comments code to v6, which has no default export: CommentMapOverlay.tsx and useCommentTool.ts now import the namespace (`import * as maplibreGl` / `import type * as maplibreGl`).
| /** The slice of a MapLibre `Map` these helpers reach through. */ | ||
| interface DynamicStyleTarget { | ||
| setPaintProperty: (layerId: string, property: string, value: unknown) => void; | ||
| getPaintProperty: (layerId: string, property: string) => unknown; | ||
| setLayoutProperty: (layerId: string, property: string, value: unknown) => void; | ||
| getLayoutProperty: (layerId: string, property: string) => unknown; | ||
| } | ||
|
|
||
| export function setDynamicPaintProperty( | ||
| map: object, |
There was a problem hiding this comment.
Minor typing nit: map is typed as bare object in all four helpers here, which is wider than the stated intent — the docstring says the goal is only to erase the property-name genericity MapLibre v6 introduced, not the map argument itself. Typing map as maplibregl.Map (or a small structural interface) instead of object would keep these from silently accepting an arbitrary non-map object while still erasing the literal-property-name checking. Low severity (internal escape hatch, not public API), but worth tightening since the module's whole point is to keep the erasure as narrow as possible.
Code reviewBugs
Security
Performance
Quality
CLAUDE.md
The rest of the migration (namespace-import rewrite across ~30 files, the |
# Conflicts: # package-lock.json # packages/map/src/layer-sync.ts # packages/map/src/map-controller.ts
|
Only the one file. That's the full scope of what I found worth flagging. Here's the final review. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
|
Updates the three workspace dependencies and the root override that forces transitive consumers onto v6. 6.1.0 is still ESM-only with no default export, so the default-import shim (Vite plugin + node --test load hook) is still required and its shimmed-package list is unchanged.
|
Binary-diff files can't take inline line comments since there's no diff hunk to anchor to. I'll fold that finding into the summary instead. Code reviewBugs
Security
Performance
Quality
CLAUDE.md
Overall this is a well-scoped, carefully-documented dependency bump with good test coverage for the trickiest parts (the shim parity test, the worker-URL fix, the GeolocateControl factory indirection). The one thing worth double-checking before merge is the binary-diff anomaly in |
1 participant
Draft. Everything in #1489 that is ours is done here and verified; what remains is a policy call on the shim (below), not unfinished work.
Depends on #1509 (blocker 3), already merged.
What was blocking, and where each stands
Blocker 2 — the frontend suite ✅
Not the plugin packages: their
exportsmaps all publish a correctimportcondition. It was our own module scope. Every workspace package is"type": "module", but the rootpackage.jsonis not, so tsx compiled all 256 test files to CJS — which selects therequirecondition and loads the.cjsentries thatrequire("maplibre-gl").tests/package.jsonwith{"type": "module"}puts them in ESM scope. The 29ERR_PACKAGE_PATH_NOT_EXPORTEDfailures go to zero. Measured both ways:ERR_PACKAGE_PATH_NOT_EXPORTEDIt is a swap, not an addition, which is why it ships here and could not land ahead of the bump.
Blocker 1 — the two external packages⚠️ shimmed
@esri/maplibre-arcgisand@geoman-io/maplibre-geoman-free(both at latest) stillimport maplibregl from "maplibre-gl"in their published ESM. Rather than wait, this rewrites that to a namespace import at load time, in the three places a module can enter the tree:vite-plugins/maplibre-default-import-shim.ts— the app build.tests/hooks/maplibre-default-import-shim.mjs— a Node loader hook, becausenode --testdoes not go through Vite.optimizeDeps.exclude— the dev server's dependency optimizer runs outside the plugin pipeline and would fail before the shim is consulted.Guards so this cannot rot:
tests/maplibre-shim-parity.test.tsfails if lists 1 and 2 disagree, and the Vite plugin errors the build if a listed package stops matching — i.e. when one ships a v6-compatible build, CI tells you to delete the entry instead of silently shimming forever.This is the part worth a second opinion. The alternative is to keep this draft parked until both publish fixes. The shim is ~40 lines and self-deleting by design, but it does mean rewriting third-party code at build time.
The bug none of the gates would have caught
v6 ships its worker as a separate file, located at runtime with
new URL("./maplibre-gl-worker.mjs", import.meta.url). That is a computed string, not a static literal, so no bundler can see it: the asset was never emitted, and at runtime the URL resolved next to the hashed app chunk (/assets/maplibre-gl-worker.mjs) where nothing exists. In the web build the SPA fallback answers that request withindex.html, so it does not even 404 — the worker is handed HTML and the request hangs.The map still drew, so build, typecheck, and most of e2e were all green. Tile parsing was quietly degraded: in the same 25-second window, 39 tile requests before the fix, 68 after. It surfaced only because
pwa.spec.tswaits fornetworkidle, which that hanging request never let happen.Fixed by importing the worker through Vite (
?worker&url, so its./maplibre-gl-shared.mjsimport is bundled too) and pointingsetWorkerUrlat the emitted asset.Everything else
import * as maplibregl from "maplibre-gl".overridespin —@maplibre/maplibre-gl-directionsstill peersmaplibre-gl@^5.0.0, which otherwise resolves a second hoisted copy and makesMapa different nominal type across package boundaries (the real cause of the originalSwipeControl is not assignable to IControlerrors). One copy installs now.packages/map/src/dynamic-style-property.ts— v6 madeset/getPaintPropertyandset/getLayoutPropertygeneric overkeyof AllPaintProperties. Property names we compute at runtime (Object.entries(spec.paint),`${prop}-transition`) need a cast; they live in one documented module instead of scattered call sites.gm:*events —Map#on/off's catch-all overload narrowed fromtype: stringtokeyof MapEventType.GeoAgentSyncableTools— its map slice now uses method syntax, so a realMapstill satisfies it understrictFunctionTypeswhile test fakes still do.map-controller.test.tsswappedmaplibregl.GeolocateControlto inject a fake, which the sealed v6 namespace rejects (same root cause as fix(map): patch Popup.prototype instead of the maplibre namespace #1509). Construction now goes through a factory the test can override.nodeexport condition resolves a CJS bundle whose default is the module object, not the constructor. Only surfaced once the tests moved to ESM scope; the browser build was always fine.Verification
test:frontend4236 passed / 0 failed; coverage 86.93 lines / 84.15 branches / 71.13 functions, all above the floors.tsc -b && vite buildclean ·lint0 errors ·test:workerclean.@esri/maplibre-arcgisinstantiates through it with all 8 exports intact.test:e2e19/23 locally. All four failures (a11y, bothexportspecs,layer-panel) reproduce on this machine independent of this branch and each passes in isolation — they look like local resource contention, and CI is the tiebreaker.Not run locally: backend/rust suites, which this cannot affect.