Updated svg map library (CJS>ESM)#29139
Conversation
WalkthroughChangesThis PR replaces the Related Issues
Suggested labels: Suggested reviewers: none identified from provided context Poem 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ 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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run @tryghost/admin-x-settings:test:acceptance |
✅ Succeeded | 11m 2s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | <1s | View ↗ |
nx run-many -t test:unit -p @tryghost/shade,@tr... |
✅ Succeeded | 7m 15s | View ↗ |
nx run ghost:test:integration |
✅ Succeeded | 2m 48s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 4m 36s | View ↗ |
nx run ghost:test:ci:integration |
✅ Succeeded | 4s | View ↗ |
nx run ghost:test:legacy |
✅ Succeeded | 3m 1s | View ↗ |
nx run ghost-admin:test |
✅ Succeeded | 2m 53s | View ↗ |
Additional runs (7) |
✅ Succeeded | ... | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-07-06 19:15:32 UTC
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #29139 +/- ##
==========================================
- Coverage 74.29% 74.26% -0.03%
==========================================
Files 1565 1565
Lines 135738 135738
Branches 16459 16447 -12
==========================================
- Hits 100845 100806 -39
- Misses 33864 33902 +38
- Partials 1029 1030 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ref https://linear.app/ghost/issue/PLA-96/spike-rolldown-vite-drop-in-build-benchmarks country-flag-icons is true ESM, so the Flag component no longer needs the CJS interop shim added in #29137 to work around react-world-flags's {__esModule, default} shape under Rolldown; supersedes #29137.
9larsons
force-pushed
the
steve/pla-96-a-flag-esm-swap
branch
from
fd2eedf to
632ee13
Compare
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/shade/src/components/ui/flag.tsx (1)
33-45: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider passing an accessible name to the rendered flag SVG.
FlagIconis rendered without atitle, so the icon has no accessible name for screen readers.country-flag-iconsReact components support atitleprop for this purpose.♻️ Optional accessibility improvement
<FlagIcon + title={code} className='absolute w-auto max-w-none rounded-[2px]'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/shade/src/components/ui/flag.tsx` around lines 33 - 45, The FlagIcon rendered in the flag component has no accessible name, so screen readers cannot identify it. Update the FlagIcon usage in the flag component to pass a title prop for the country name/label, using the existing FlagIcon render path and keeping the defaultFallback unchanged. Use the FlagIcon symbol in this component to locate the render site and ensure the accessible name is provided whenever the SVG is rendered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/shade/src/components/ui/flag.tsx`:
- Line 31: The fallback handling in Flag is treating an explicit null the same
as an omitted value, so `fallback={null}` still renders the default placeholder.
Update the `Flag` component’s fallback selection logic to distinguish “not
provided” from “intentionally empty” by checking for `undefined` only, and
preserve `null` as a valid value that renders nothing. Use the existing
`FlagProps.fallback` and `defaultFallback` code path in `flag.tsx` to apply the
fix.
---
Nitpick comments:
In `@apps/shade/src/components/ui/flag.tsx`:
- Around line 33-45: The FlagIcon rendered in the flag component has no
accessible name, so screen readers cannot identify it. Update the FlagIcon usage
in the flag component to pass a title prop for the country name/label, using the
existing FlagIcon render path and keeping the defaultFallback unchanged. Use the
FlagIcon symbol in this component to locate the render site and ensure the
accessible name is provided whenever the SVG is rendered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 37ed421f-b800-4dc8-b5dd-1b53ec53ea67
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
apps/shade/package.jsonapps/shade/src/components/ui/flag.stories.tsxapps/shade/src/components/ui/flag.tsxpnpm-workspace.yaml
| const code = countryCode ? countryCode.toUpperCase() : ''; | ||
| const FlagIcon = code && hasFlag(code) ? (Flags as Record<string, FlagComponent>)[code] : undefined; | ||
|
|
||
| const defaultFallback = fallback || <span className='h-[14px] w-[22px] rounded-[2px] bg-muted-foreground/20' style={sizeStyle}></span>; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
fallback of null doesn't render nothing — it still falls back to the default placeholder.
FlagProps.fallback is typed as React.ReactNode | null | undefined, implying null is a distinct, intentional value (e.g. "render nothing"). But fallback || <span .../> treats null the same as undefined since both are falsy, so an explicit fallback={null} still renders the default muted placeholder instead of nothing.
🐛 Proposed fix distinguishing "not provided" from "explicitly empty"
- const defaultFallback = fallback || <span className='h-[14px] w-[22px] rounded-[2px] bg-muted-foreground/20' style={sizeStyle}></span>;
+ const defaultFallback = fallback === undefined
+ ? <span className='h-[14px] w-[22px] rounded-[2px] bg-muted-foreground/20' style={sizeStyle}></span>
+ : fallback;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const defaultFallback = fallback || <span className='h-[14px] w-[22px] rounded-[2px] bg-muted-foreground/20' style={sizeStyle}></span>; | |
| const defaultFallback = fallback === undefined | |
| ? <span className='h-[14px] w-[22px] rounded-[2px] bg-muted-foreground/20' style={sizeStyle}></span> | |
| : fallback; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/shade/src/components/ui/flag.tsx` at line 31, The fallback handling in
Flag is treating an explicit null the same as an omitted value, so
`fallback={null}` still renders the default placeholder. Update the `Flag`
component’s fallback selection logic to distinguish “not provided” from
“intentionally empty” by checking for `undefined` only, and preserve `null` as a
valid value that renders nothing. Use the existing `FlagProps.fallback` and
`defaultFallback` code path in `flag.tsx` to apply the fix.
ref https://linear.app/ghost/issue/PLA-96 - adds a `rolldown` named catalog (vite: npm:rolldown-vite@7.3.1) and points the 8 admin-surface apps (posts, stats, admin, admin-x-framework, admin-x-settings, admin-x-design-system, shade, activitypub) at catalog:rolldown - public UMD apps stay on stock vite until their own Rolldown config lands (#29141) - the known Rolldown interop breakers are already fixed on main: shade Flag ESM swap (#29139), @svg-maps/world shim (#29143), @tryghost/i18n dual-format (#29144) - verified: all 8 apps + the admin shell build clean under Rolldown; react/react-dom stay external (no dual-instance regression); only benign bundler-agnostic warnings
1 participant
Summary
Replaces the CommonJS
react-world-flagsdependency with the true-ESMcountry-flag-iconsin Shade'sFlagcomponent (apps/shade/src/components/ui/flag.tsx).This supersedes / closes #29137, which added a CJS
interopDefaultshim to work aroundreact-world-flags's{__esModule, default}CJS shape under Rolldown.country-flag-iconsships as native ESM ("type": "module",import→index.js), so no CJS interop shim is needed — the guiding principle here is "no CJS shims unless we have no choice", and this dependency gives us that choice.What changed
pnpm-workspace.yaml: addedcountry-flag-icons: 1.6.20to the catalog (respectscatalogMode: strict+minimumReleaseAge— 1.6.20 shipped 2026-07-01, well past the 3-day window).apps/shade/package.json: addedcountry-flag-icons: "catalog:"; removedreact-world-flagsand its@types/react-world-flagsdevDep (country-flag-icons ships its own types).apps/shade/src/components/ui/flag.tsx: rewritten to render flags dynamically by ISO-3166 alpha-2 code.Import approach
The
countryCodeis upper-cased (country-flag-icons keys are uppercase), guarded withhasFlag(code), and the component is looked up asFlags[code]. All native ESM imports — nointeropDefault, no CJS shim.Preserved API
The
FlagPropsAPI is unchanged:width,height,className,countryCode,fallback. The outer<div>wrapper,sizeStyle, rounded styling, and the default muted-placeholder fallback span are all preserved.Invalid / missing code handling
If
countryCodeis empty, orhasFlag(code)is false (invalid/unknown code, e.g.XX), the component renders thefallbackprop when provided, otherwise the default muted placeholder span — same behaviour the oldfallbackprop gave.Bundle-size delta
es/output is unchanged (~1.6M total;flag.js893 B → 1085 B). Shade externalizes all bare-specifier imports invite.config.ts, socountry-flag-iconsis not bundled into Shade — it's a runtime dependency resolved by the consuming app.Flags[code]), Rollup cannot tree-shake, so the wholecountry-flag-icons/react/3x2barrel is pulled in: ~324 KB raw / ~57 KB gzipped for all 265 flags.This is meaningful but not egregious (not multiple MB). If we want to trim it later, the lighter alternative is per-code dynamic
import()(e.g.import(\country-flag-icons/react/3x2/${code}`)`) so only the flags actually rendered are loaded — at the cost of an async/Suspense boundary in the component. Kept the straightforward synchronous version here.Validation (stock Vite 7.3.2 + Rollup)
pnpm install— clean (+1 -8); lockfile has noreact-world-flagsrefs.pnpm --filter @tryghost/shade build— passes (type-check + vite build); builtflag.jsreferencescountry-flag-iconsas external ESM imports, noreact-world-flags.pnpm --filter @tryghost/shade lint— passes.pnpm --filter @tryghost/shade test:unit— 226 tests pass.Draft
Draft so a human can eyeball actual flag rendering (valid codes, invalid
XX, custom fallback) before finalizing.