Repository navigation
Conversation
EMDASH_E2E_BUILD=production now builds the Node fixture with `astro build --devOutput` and serves dist/server/entry.mjs instead of running `astro dev`. The build keeps `import.meta.env.DEV` true, so the dev-bypass, dev-reset and captured-email endpoints the suite depends on work without any test switch in EmDash. Also fixes what a built fixture exposes: manual i18n routing so the fixture's [locale] pages win over Astro's fallback routes, sharp for the image endpoint, the registry fixture installed at server start, an https image URL for the CSP, and an ignore rule for the migrations journal that builds write into the fixture. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The build-time registry check only allowed localhost for `astro dev` and `astro sync`, while the runtime check follows import.meta.env.DEV. A --devOutput build therefore failed at build time although the built site would accept the URL. Astro sets NODE_ENV=development for --devOutput before loading the config, so the build check uses that. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
🦋 Changeset detectedLatest commit: 837c119 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
|
All contributors have signed the CLA ✍️ ✅ |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
The opt-in built-fixture approach is a reasonable way to speed up the Node E2E shards. It keeps the dev-only auth bypass working through astro build --devOutput, and it leaves Cloudflare and the dev-server path unchanged. Both reviewers found the fixture, setup/teardown and changeset plumbing sound, and neither ran anything, so the timing numbers and suite results are unverified.
Both reviewers flagged the same line in core, packages/core/src/astro/integration/index.ts:638, as needing a fix. The change loosens the build-time registry URL policy so that HTTP and localhost aggregators are accepted when NODE_ENV === "development". That check has two problems:
- It uses
process.env.NODE_ENV, which the repository's AGENTS.md forbids (raised by one reviewer). - No unit test guards the new rule or the changeset's promise that production builds still reject these URLs (raised by both).
The E2E run exercises only the accepting side, and only indirectly.
Reviewed by anthropic/claude-opus-5-5 and openai/gpt-6.1-sol.
| // `astro build --devOutput` sets NODE_ENV before loading the config and | ||
| // produces a build whose runtime registry check allows localhost. | ||
| allowLocalhost: | ||
| command === "dev" || command === "sync" || process.env.NODE_ENV === "development", |
There was a problem hiding this comment.
[needs fixing] This line changes the build-time registry URL policy. HTTP and localhost aggregators are now accepted when process.env.NODE_ENV === "development" during any command, not only dev and sync. Two problems:
-
Convention (raised by one reviewer, not the other): AGENTS.md requires development/production decisions to use Astro/Vite environment semantics and says never to use
process.env.NODE_ENV. The runtime validator inregistry/config.tsusesimport.meta.env.DEV, so this adds a second source of truth for the same policy. Determine development-output status through the appropriate Astro/Vite configuration boundary, or document an explicit config-time exception if the hook cannot access that signal. The other reviewer found the build-time and runtime checks agree in practice, since Astro defaultsNODE_ENVonly when it is unset andDEVfollows it, but did not address the AGENTS.md rule. -
No test coverage (raised by both reviewers):
tests/unit/astro/integration/already calls theastro:config:setuphook directly withcommand: "build"(seeimage-remote-patterns.test.ts), so a test is cheap to add. The changeset promises "Production builds still reject them", and nothing guards that. Add cases withregistry: "http://localhost:1234"andcommand: "build":NODE_ENVunset or"production": expectREGISTRY_AGGREGATOR_URL_FORBIDDEN.NODE_ENV = "development"(the--devOutputcase): expect the hook to succeed.
Restore
process.env.NODE_ENVafter each case.
|
I have read the CLA Document and I hereby sign the CLA |
Approval statusThe current revision does not meet the approval requirements yet.
This comment updates as reviews arrive. See MAINTAINING.md and the proposal process for the rules. |
What does this PR do?
Most of the Node E2E time goes to
astro devcompiling pages on first request. This PR runs the Node shards against a built fixture instead, usingastro build --devOutput. This is a simpler way of retaining existing methods of "dev only" functionality while providing a small speed boost.It's opt-in:
EMDASH_E2E_BUILD=productionswitchese2e/global-setup.tsto build and serve the fixture, and CI sets it only for the Node step. The Cloudflare step and the 8-shard layout are unchanged. The rest is what the fixture needs to work as a built site: manual i18n routing, asharpdependency, the registry fixture installed from middleware, and anhttps:image URL in one test. There is also one small core fix. The build-time registry check rejected the suite's localhost mock marketplace for--devOutputbuilds, even though the built site accepts it at runtime.astro dev(main)astro build --devOutput(this PR)A real production build is a lot faster (2.8 min), but it needs a new config option; that's #4519. For a full local run you also need the test fixes in #4517. I also tried this build on Endform, which runs the whole suite in one job against a single server (endformdev#8). Under that load the dev-output server times out, and the production build doesn't.
Honestly I think #7 is better - long term this will provide a more stable suite, if you accept the tradeoffs.
I used claude to help me create this PR, but I have run the suite and reviewed the code myself.
Type of change
Your connection to this change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change): full Node E2E suite against the dev-output build (344 passed, 34 skipped)pnpm formathas been runAI assistance disclosure
Screenshots / test output
Not applicable.