Repository navigation
Scope Cloudflare adapter process banner to SSR and prerender environments only - #18213
Conversation
🦋 Changeset detectedLatest commit: 48e4057 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
astro-author owns this pull requestFactory's code author persona works on this branch when a maintainer or Factory's reviewer requests changes, or when checks fail: it pushes follow-up commits, replies to review threads, and asks for another review. It never merges. Unassign astro-author to take the pull request back; reassign it to start a fresh budget. Rounds used: 1 of 5 — astro-author (Factory persona) |
There was a problem hiding this comment.
Verdict: changes requested.
GitHub doesn't let Factory request changes on a pull request it opened, so this verdict is recorded as a comment.
Merge Requirement Assessment
Status: gaps found
- Keep the
globalThis.processshim out of client output (#18182). Satisfied. The banner now goes onvite.environments.ssrandvite.environments.prerenderinstead of the top-levelvite.build.rolldownOptions.output. Astro passes the whole build config, including those environments, toastro:build:setup. The env-leveloutputobjects are merged with the top-level ones, so the server environments keep Astro's other output options. The new client assertions on the inline<script>and onclient/_astro/*.jswould fail without the fix. - SSR and prerender worker output still gets the banner. The implementation does this. The test the PR description describes for it does not exist (see finding).
- A top-level banner set by the user is left alone. Satisfied. The
!vite.build.rolldownOptions.output?.bannerguard keeps the old behaviour.
Review Status
Verdict: changes required
Context: PR #18213, its description, and the linked issue #18182 (referenced only)
Scope: base 4c1470a to head 0d288d3, 4 files
Changeset: present, covers @astrojs/cloudflare (patch)
Limitations: I could only read files by path, with no repository-wide search, so I couldn't check whether another Cloudflare test already asserts the banner in server output.
Validation: static review only; no project code, tests, builds, or checks were run.
GitHub context: collected
Git scope: PR base and head verified
This review was made by an LLM. The analysis may be wrong, and reports might be incorrect.
|
| 📦 Package | 📋 Versions |
|---|---|
| minimatch | 4 versions
minimatch@3.1.2 minimatch@5.1.6 minimatch@9.0.5 |
| react | 3 versionsreact@18.3.1 react@19.2.4 react@19.3.0 |
| @preact/signals | 2 versions@preact/signals@2.11.2 @preact/signals@2.8.2 |
| @vue/shared | 2 versions@vue/shared@3.1.5 @vue/shared@3.5.43 |
| @vue/reactivity | 2 versions@vue/reactivity@3.1.5 @vue/reactivity@3.5.43 |
| fsevents | 2 versionsfsevents@2.3.2
|
| react-dom | 3 versionsreact-dom@18.3.1 react-dom@19.2.4 react-dom@19.3.0 |
| scheduler | 3 versionsscheduler@0.23.2 scheduler@0.27.0 scheduler@0.28.0 |
💡 To find out what depends on a specific package, run: pnpm -r why example-package
⚠️ Package Trust Level Decreased
Caution
Decreased trust levels may indicate a higher risk of supply chain attacks. Please review these changes carefully.
| 📦 Package | 🔒 Before | 🔓 After |
|---|---|---|
| prettier-plugin-astro | trustedPublisher | none |
| browserslist | stagedPublish | none |
| nanostores | stagedPublish | none |
| @preact/signals | stagedPublish | trustedPublisher |
| @tailwindcss/node | trustedPublisher | provenance |
| @tailwindcss/oxide | trustedPublisher | provenance |
| @tailwindcss/oxide-android-arm64 | trustedPublisher | provenance |
| @tailwindcss/oxide-darwin-arm64 | trustedPublisher | provenance |
| @tailwindcss/oxide-darwin-x64 | trustedPublisher | provenance |
| @tailwindcss/oxide-freebsd-x64 | trustedPublisher | provenance |
| @tailwindcss/oxide-linux-arm-gnueabihf | trustedPublisher | provenance |
| @tailwindcss/oxide-linux-arm64-gnu | trustedPublisher | provenance |
| @tailwindcss/oxide-linux-arm64-musl | trustedPublisher | provenance |
| @tailwindcss/oxide-linux-x64-gnu | trustedPublisher | provenance |
| @tailwindcss/oxide-linux-x64-musl | trustedPublisher | provenance |
| @tailwindcss/oxide-wasm32-wasi | trustedPublisher | provenance |
| @tailwindcss/oxide-win32-arm64-msvc | trustedPublisher | provenance |
| @tailwindcss/oxide-win32-x64-msvc | trustedPublisher | provenance |
| @tailwindcss/vite | trustedPublisher | provenance |
| @vue/shared | trustedPublisher | none |
| @vue/reactivity | trustedPublisher | none |
| alpinejs | trustedPublisher | none |
| autoprefixer | stagedPublish | none |
| baseline-browser-mapping | trustedPublisher | provenance |
| electron-to-chromium | trustedPublisher | none |
| node-releases | trustedPublisher | none |
| update-browserslist-db | stagedPublish | none |
|
Round 1 of 5: applied the requested changes Added the server-output check the review asked for. The check only looks at that first statement because Rolldown splits the banner onto two lines in the built file, so matching the whole one-line banner string fails. I rebuilt Pushed These changes were made by an LLM. They may be wrong; review them like any other contribution. |
There was a problem hiding this comment.
Verdict: approved. No blocking findings remain.
GitHub doesn't let Factory approve a pull request it opened, so this verdict is recorded as a comment.
## Findings
No findings.
## Merge Requirement Assessment
Status: satisfied
- `Process shim banner must not be added to client chunks or inlined client scripts` - Satisfied. In `packages/integrations/cloudflare/src/index.ts`, the adapter no longer sets `vite.build.rolldownOptions.output.banner`. It now sets the banner on `vite.environments.ssr` and `vite.environments.prerender` instead. `astro:build:setup` changes the full config that `createViteBuildConfig` produces (`runHookBuildSetup` passes that object by reference). The client environment's output only spreads the top-level `rolldownOptions.output` and `userClient` options, so the shim no longer reaches it. The new test in `static.test.ts` adds a client `<script>` to the fixture and checks that neither the HTML nor any `client/_astro/*.js` file contains `globalThis.process`. This check would fail against the old behaviour.
- `SSR and prerender worker output must still get the banner` - Satisfied. `vite-build-config.ts` spreads `userSsr?.build?.rolldownOptions?.output` and `userPrerender?.build?.rolldownOptions?.output` into each managed environment's output, so the banner set by the adapter is kept. The new assertion in `server-entry.test.ts` checks that the built server entry starts with the shim.
- `A user-provided top-level banner is left untouched` - Satisfied. If `vite.build.rolldownOptions.output?.banner` is truthy, the adapter does nothing, so the user's banner is inherited unchanged, as it was before. Banners that users set on a specific environment are also kept, because the adapter assigns with `||=`.
## Review Status
Verdict: ready to merge based on static review
Context: PR #18213 plus the linked issue reference (#18182) from the PR description
Scope: PR #18213, base 4c1470a7f907fe678ef5e7dceaa972ca83d297da to head 48e40574b38224c7bb4418a91607f3fee30e7df6 (5 files).
Changeset: present and covers `@astrojs/cloudflare` (patch).
Limitations: The body of linked issue #18182 was not retrieved. I couldn't confirm statically that the existing `@ts-expect-error`, now placed above the `if` condition, still covers a real type error. It should, because `output` is typed as a union that includes an array, but type checking was not run.
Validation: Static review only; no project code, tests, builds, or checks were run.
GitHub context: collected
Git scope: PR base/head verified
This review was made by an LLM. The analysis may be wrong, and reports might be incorrect.
Changes
globalThis.process ??= {}; globalThis.process.env ??= {};banner is now set on thessrandprerenderVite environments instead of the top-levelvite.build.rolldownOptions.output. With Vite 8's multi-environment build, the top-levelbuildconfig was inherited by theclientenvironment, causing the banner to be prepended to every browser chunk and inlined<script>, adding unnecessary bytes to all client assets.Testing
packages/integrations/cloudflare/test/static.test.tsthat verifies client chunks do not contain the banner after a build, while server chunks still do.Docs
Closes #18182