fix: limit admin source aliasing to repo dev#183
Conversation
🦋 Changeset detectedLatest commit: 53cce68 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
There was a problem hiding this comment.
Pull request overview
Updates the Astro integration’s Vite configuration so that, during astro dev, EmDash uses the built @emdash-cms/admin package by default (instead of forcing raw source), with an opt-in escape hatch via EMDASH_USE_ADMIN_SOURCE=1.
Changes:
- Gate admin source aliasing in dev behind
EMDASH_USE_ADMIN_SOURCE=1, otherwise use admindist/. - Keep additional SSR dependencies (
sanitize-htmlparsing stack) external for Node SSR to avoid runtime ESM interop issues. - Add unit tests for SSR externals composition and admin alias selection.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/core/src/astro/integration/vite-config.ts | Switch dev default to admin dist/, add NODE_SSR_EXTERNALS, and apply it to SSR externals + optimizeDeps exclusions. |
| packages/core/src/astro/routes/admin.astro | Change AdminWrapper import to a local sibling module. |
| packages/core/tests/unit/astro/vite-config.test.ts | New tests covering SSR externals behavior and admin aliasing behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e5a0089 to
d620370
Compare
|
Can you explain a bit about why you're suggesting this change? I would rather just do a fix to ensure it doesn't apply when installed for end users. |
|
Thanks — agreed. I was trying to keep the dev-only admin source alias from leaking into installed apps, but the first pass was too broad. The main use case is external Astro apps consuming EmDash — including linked/local installs — where the host app should see the same built admin package surface it would get from npm, not raw admin source. I pushed a narrower version that keeps source aliasing for local repo development and uses the built admin package for external app installs. I also dropped the unrelated SSR externalization and |
|
We should probably just not be shipping the src folder to production |
|
Thanks @ascorbic Agreed — that sounds like the cleanest fix. One nuance from checking the manifests: The source-shipping issue is in I can keep this PR scoped as a small guard for consumer apps, then do a follow-up to move those exports to built outputs and remove Happy to tackle that next if that’s the direction you want. |
|
Most of those UI exports are Astro components though, which are not compiled |
1e1598b to
9fa6698
Compare
|
Narrowed this to the admin package aliasing behavior only; no |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const isDev = command === "dev"; | ||
| const projectRoot = fileURLToPath(options.astroConfig.root); | ||
|
|
||
| // In dev mode within the monorepo, alias JS imports to source for instant HMR. | ||
| // CSS always comes from dist/ (pre-compiled by @tailwindcss/cli) since Tailwind's | ||
| // Vite plugin has native deps that don't bundle well. Run `pnpm dev` in packages/admin | ||
| // alongside the demo server to get CSS watch-rebuilds too. | ||
| const adminSourcePath = isDev ? resolveAdminSource() : undefined; | ||
| const adminSourcePath = isDev ? resolveAdminSource(projectRoot) : undefined; | ||
| const useSource = adminSourcePath !== undefined; | ||
|
|
There was a problem hiding this comment.
useSource doesn't narrow adminSourcePath for TypeScript, so adminSourcePath remains string | undefined even when useSource is true. Consider structuring this as a direct check on adminSourcePath (or typeof adminSourcePath === "string") so later uses can be type-safe without relying on non-local reasoning.
| // and redirect locale .mjs imports to dist/. | ||
| // In production, macros are pre-compiled by tsdown in the admin package. | ||
| ...(useSource ? [linguiMacroPlugin(adminSourcePath!, adminDistPath)] : []), | ||
| ...(useSource ? [linguiMacroPlugin(adminSourcePath, adminDistPath)] : []), |
There was a problem hiding this comment.
This call passes adminSourcePath (typed string | undefined) into linguiMacroPlugin (expects string). Even though useSource is derived from adminSourcePath !== undefined, TypeScript won't correlate them; please either (a) branch on adminSourcePath directly here, or (b) reintroduce a local non-null assertion after narrowing so the plugin always receives a string.
| ...(useSource ? [linguiMacroPlugin(adminSourcePath, adminDistPath)] : []), | |
| ...(useSource ? [linguiMacroPlugin(adminSourcePath!, adminDistPath)] : []), |
| const externalProjectRoot = new URL("file:///workspace/emdash-site/"); | ||
| const siblingProjectRoot = new URL("../../../../../../emdash-site/", import.meta.url); |
There was a problem hiding this comment.
The test hard-codes file:///workspace/emdash-site/ as the external project root. This path is environment-specific and can also be an invalid file URL on Windows (no drive/UNC), making the test less portable. Prefer constructing an external root via pathToFileURL(resolve(os.tmpdir(), ...)) or another deterministic path derived from the current runtime.
|
/ultrareview |
|
All concentrated in one file. The PR is clean. Per the instructions: "If the PR looks good, respond with only 'LGTM!' and skip posting a review." LGTM! |
What does this PR do?
Fixes Astro dev so the raw
@emdash-cms/adminsource alias only applies when running an EmDash app inside this repo. External app installs now use the built admin package surface, while local monorepo/admin development keeps source aliasing for HMR.The tests cover:
packages/admin/srcpackages/admin/distdistType of change
Checklist
pnpm typecheckpassespnpm --silent lint:json | jq '.diagnostics | length'returns 0pnpm testpasses (or targeted tests for my change)pnpm formathas been runAI-generated code disclosure
Screenshots / test output
pnpm --silent lint:quickpnpm --filter emdash exec vitest run tests/unit/astro/vite-config.test.tspnpm typecheck