refactor: split platform-specific code into platform-vercel and platform-cloudflare packages#8838
refactor: split platform-specific code into platform-vercel and platform-cloudflare packages#8838
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This comment was marked as outdated.
This comment was marked as outdated.
👋 Codeowner Review RequestThe following codeowners have been identified for the changed files: Team reviewers: @nodejs/web-infra @nodejs/nodejs-website Please review the changes when you have a chance. Thank you! 🙏 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8838 +/- ##
==========================================
- Coverage 73.87% 73.85% -0.02%
==========================================
Files 105 105
Lines 8883 8882 -1
Branches 326 325 -1
==========================================
- Hits 6562 6560 -2
- Misses 2320 2321 +1
Partials 1 1 ☔ View full report in Codecov by Sentry. |
|
FYI deployment will only work once this gets merged as we need to change the root directory within vercel. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
avivkeller
left a comment
avivkeller
left a comment
There was a problem hiding this comment.
Really really against the symlinks, so blocking on that regard
Why so? And what are your alternatives? Because Im unsure if there are any... Without duplicating every file 😅 |
|
Aviv, this PR is a PoC no need to go super nitpicky on it. I haven't done myself a round of reviews yet as I just published this PR so we can start analyzing approaches, I might as well convert to a draft. |
Noted, I've converted it to a draft so it's clearer for reviewers |
I'm not sure what the alternatives are, but Symlinks are a, in my opinion, slippery slope of inconsistent behavior across operating systems and build tools. Is there a way to tell Next.js / OpenNext to look at |
I'm investigating 👀 |
Platform detection in apps/site was spread across three different mechanisms: `VERCEL_ENV`, `OPEN_NEXT_CLOUDFLARE`, and a runtime `'Cloudflare' in global` check in the MDX plugin. Each one carried its own caveats (the global check blocked tree-shaking; the two env vars couldn't be compared to "is this build neutral / Vercel / Cloudflare?" uniformly), and Vercel analytics were imported eagerly in the root layout even on Cloudflare builds. Replace all three with a single `NEXT_PUBLIC_DEPLOY_TARGET` env var set by each deployment wrapper (`vercel.json` -> `vercel`, `open-next.config.ts` -> `cloudflare`). The `NEXT_PUBLIC_` prefix lets Next.js inline the value at build time so platform-specific branches are dead-code-eliminated from non-matching bundles. Extract the Vercel Analytics + SpeedInsights injection into a `platform/body-end` slot. The core layout renders `<BodyEnd />` and the slot's dynamic import resolves only on Vercel builds, so the Vercel modules no longer ship to Cloudflare.
ovflowd
force-pushed
the
refactor/split-apps-by-deployment-target
branch
from
659dda2 to
d7b6e7e
Compare
ovflowd
changed the title
…orm-cloudflare packages
Why: Upgrading Next.js or any Vercel dep was gated by OpenNext's
compatibility window, platform-specific runtime branches
(VERCEL_ENV, OPEN_NEXT_CLOUDFLARE, 'Cloudflare' in global) were
scattered across the codebase, and apps/site carried deps that only
one deployment used.
This extracts all platform-specific integrations into dedicated
workspace packages selected at build time via NEXT_PUBLIC_DEPLOY_TARGET:
- @node-core/platform-vercel owns Vercel analytics, speed insights,
and OTel instrumentation.
- @node-core/platform-cloudflare owns the OpenNext config, Wrangler
config, worker entrypoint (with Sentry), image loader, and the MDX
flags needed to skip WASM/Twoslash on Cloudflare workers.
Each adapter exports a next.platform.config.mjs with a
{ nextConfig, aliases, images, mdx } contract that apps/site merges
into its Next.js config, MDX plugins, and Playwright config via
dynamic import. A no-op apps/site/next.platform.config.mjs and
apps/site/playwright.platform.config.mjs keep the standalone
pnpm dev / pnpm build paths working when no target is set.
Runtime platform detection (PLAYWRIGHT_RUN_CLOUDFLARE_PREVIEW,
'Cloudflare' in global, OPEN_NEXT_CLOUDFLARE branches) is replaced
with NEXT_PUBLIC_DEPLOY_TARGET selection so the apps/site source
tree has no platform conditionals left.
Docs updated: docs/technologies.md documents the
NEXT_PUBLIC_DEPLOY_TARGET contract; docs/cloudflare-build-and-deployment.md
points at the new package paths; CODEOWNERS moved the Wrangler /
OpenNext ownership to the new packages.
ovflowd
changed the title
Why: Turbopack's `resolveAlias` treats absolute paths as server-relative (prepending `./`) and rejects them with "server relative imports are not implemented yet". The `fileURLToPath(new URL(...))` pattern produced absolute paths that broke the plain `pnpm build` CI job. Project-relative strings resolve correctly in both Turbopack and webpack.
…le site deps Platform packages are internal deployment wrappers, not publishable artifacts. Moving them out of packages/ keeps the publish workflow's globs untouched and matches the repo convention (apps/* = internal, packages/* = published). Also removes @node-core/platform-vercel and @node-core/platform-cloudflare from apps/site's dependencies. They're now declared as optional peer dependencies so: - A standalone install (no DEPLOY_TARGET) pulls in zero platform deps. - Vercel's installCommand scopes install to @node-core/website and @node-core/platform-vercel (skipping Cloudflare/OpenNext deps). - The Cloudflare deploy workflow scopes install to @node-core/website and @node-core/platform-cloudflare (skipping @vercel/* deps). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…lare The platform adapter sub-trees moved out of packages/ in the previous commit. This refreshes the tree diagram so docs aren't misleading for new contributors. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ontract Replace the template-literal `await import()` pattern in next.config.mjs, mdx/plugins.mjs, and playwright.config.ts with a static `import from '#platform/...'` resolved by Node's subpath imports. Each platform's build command sets `NODE_OPTIONS=--conditions=<target>` so the imports map selects the matching @node-core/platform-* package; without the flag, the site's local no-op file is used. Consolidate the duplicated PlatformConfig / PlatformPlaywrightConfig types into apps/site, the contract owner. Platform .mjs files JSDoc the site types via relative path; platform-side .d.ts duplicates are removed. Also: arrow-function instrumentation hooks; prune the redundant PLAYWRIGHT_BASE_URL env from the Cloudflare Playwright workflow and config (kept in the Vercel workflow where it carries the dynamic preview URL).
Turbo's persistent-task output buffering swallows wrangler dev's readiness signal in CI, making Playwright's webServer probe time out at 180s even though the preview eventually comes up. Move the OpenNext build to a dedicated CI step so Playwright's webServer can invoke wrangler dev directly, keeping stdout attached to the CI log and removing the dependsOn chain indirection.
wrangler's bin is hoisted to apps/cloudflare/node_modules/.bin under pnpm, not repo-root node_modules/.bin, so the previous direct path failed with ENOENT in CI. Delegating to the package script via `pnpm --filter` resolves wrangler from the right workspace and keeps stdout attached (no turbo buffering).
dario-piotrowicz
left a comment
dario-piotrowicz
left a comment
There was a problem hiding this comment.
I appreciate where the PR is coming from, however I don't think I am a huge fan of this, having 3 apps being cloudflare, vercel and site seems quite unclear and unintuitive to me (since there is only a single app, site which can have 2 different variations). There are also magic build time resolutions that I feel are very likely to confuse and alienate people not too familiar with the codebase (and me 😅).
Sorry but personally I feel like, instead of making things clearer the changes here introduce clear/strong boundaries between the different versions of the site at the cost of making the project less clear and navigatable (and approachable by newcommers).
If it were me and we wanted to isolate the vercel and cloudflare logic using different packages in the monorepo I think I'd try to keep site being the only app and instead add cloudflare and vercel as actual packages instead and having site depend on those (with some minor exceptions such as wrangler and open-next config files).
Sorry this is my general take on the approach 😓
(PS: if people are happy with this I'll definitely won't try to spot this direction though 🙂)
Webpack bundles the top level of each platform's next.platform.config.mjs into the server output (because apps/site/mdx/plugins.mjs reads .mdx from it), which meant the Cloudflare build shipped `createRequire(...).resolve(...)` and `await getDeploymentId()` into the worker runtime — failing at request time with `m.resolve is not a function`. Moving the heavy, build-only pieces (`@opennextjs/cloudflare` imports, `require.resolve`, VERCEL_URL computation) behind async thunks keeps the module's top level free of Node-runtime-only code. Pair with the webpack `conditionNames` wiring so `#platform/*` actually resolves to the target variant at bundle time.
This is the exact opposite others asked. To be clear, apps/cloudflare and apps/vercel aren't "apps" or variations of the website, they are "packages", they are under "apps" because @avivkeller asked, as everything under "packages/" is supposed to be published. I'm fine making a new root folder named "platforms" To also clarify here, @dario-piotrowicz, apps/site is the only package, all that happens is that when building for Vercel or for Cloudflare we tell Node.js to load the platform-specific config files and things that each one of these environments used. |
Oh ok... different people have different preferences 🙈
I see... yeah probably platforms would be clearer in my opinion 🤔 , although they are both tied to the
Yes I got that, but the dir structure really doesn't make it clear in my opinion, since |
… dir Replace `cd ../site && ...` in the three cloudflare scripts with `pnpm --filter=@node-core/website exec ...` so pnpm sets cwd transparently (per Dario's review feedback). Ignore apps/cloudflare/.wrangler, which is where wrangler v4 writes its state dir relative to the config file. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Im unsure how putting them as subfolders of apps/site makes things better. One of the goals here is that apps/site is the Next.js App, it is agnostic to any platform. But it is wired to support "platform" overrides, that's where the env flags come in, pretty much we're using Node.js to say "for cloudflare, please load the overrides that come from this apps/cloudflare" package. We aren't really building things within apps/cloudflare, nor apps/cloudflare is an actual app, I guess the name apps/ is the confusing factor. Think of apps/cloudflare or apps/vercel as bindings. Im fine moving them under another folder, but I'd like to avoid keep moving them around as @avivkeller said apps/ you said something else, so if we can reach an agreement of preference, that'd be great.
I get that. Anyhow, the goal of this PR is to:
|
Agreed 👍 , I do think that
Yes and I don't disagree that it's doing that well. My concern here is that in order to satisfy those 3 goals the structure of the monorepo is becoming (at least in my opinion) more complex, less intuitive (and more unconventional?). So I think there is a tradeoff to be aware of between isolating the platforms bits and the complexity of the repo's setup. This might also be subjective and people might not find the updated structure more complex 🤔 |
|
In general, I'm in support of isolating the deployment targets from the rest of the code as best we can, and want to keep thinking about how best to do this. I suspect we need to put the branching logic somewhere, and I am thinking through the best options:
Naming things is hard, of course, and we are not constrained by the two monorepo directories we have now. Perhaps moving them to platforms/ helps ever so slightly. At the end of the day, the READMEs are doing the work.
I would agree with this in principle but also think this approach has made it simpler in a way to casual contribution.
Maybe yes, maybe no. Is it slightly unconventional? I think it's pushing the boundaries of how apps can be constructed. Most apps don't have the luxury? the design goal? of deploying multiple ways. But this feels like a good use of a monorepo and perhaps a slightly clever way to use import maps...but one that I see as innovative 😄 I do think we should always try to optimize for casual contribution, which I think this still does via a simpler /apps/site |
bmuenzenmeyer
left a comment
bmuenzenmeyer
left a comment
There was a problem hiding this comment.
I am a 👍 on this proposal.
Due to the nature of the change, I wonder if we should bring this up in a web meeting?
There was a problem hiding this comment.
Splitting by platform also opens the door to more nuanced CODEOWNERS - if we have experts or partners in one platform or the other
Move apps/{vercel,cloudflare} to platforms/{vercel,cloudflare} and
apps/site/vercel.json to platforms/vercel/vercel.json. Rebase all
relative path references across workflows, CODEOWNERS, docs, and
configs so tooling resolves from the new locations.
vercel.json now declares outputDirectory (../../apps/site/.next) so
Vercel can find the Next.js build when its Root Directory is set to
platforms/vercel, and uses build.env for NEXT_PUBLIC_DEPLOY_TARGET and
NODE_OPTIONS=--conditions=vercel instead of cross-env (which is not in
the --prod install tree).
Add NEXT_PUBLIC_DEPLOY_TARGET and NODE_OPTIONS=--conditions=cloudflare
to the Cloudflare deploy workflow's build and deploy step env blocks
so the outer opennextjs-cloudflare process resolves imports under the
cloudflare condition (matching the Playwright workflow).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The prior commit accidentally dropped this test during the directory rename rather than moving it alongside the other platforms/vercel files. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@bmuenzenmeyer I made the changes for vercel.json, Vercel build will fail till I move change the appRoot within the UI. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 68b5524. Configure here.
| env: { | ||
| NEXT_PUBLIC_BASE_URL: | ||
| process.env.NEXT_PUBLIC_BASE_URL || VERCEL_URL || '', | ||
| }, |
There was a problem hiding this comment.
Vercel platform sets empty string as BASE_URL fallback
Low Severity
When neither NEXT_PUBLIC_BASE_URL nor VERCEL_URL is set, the env config explicitly inlines NEXT_PUBLIC_BASE_URL as '' (empty string) into the build. While BASE_URL in next.constants.mjs handles this via ||, any code that checks process.env.NEXT_PUBLIC_BASE_URL for truthiness vs. existence will behave differently than when the variable is simply absent. Using undefined instead of '' as the fallback would be more consistent and avoid masking the "unset" state.
Reviewed by Cursor Bugbot for commit 68b5524. Configure here.
Extracts all Vercel- and Cloudflare-specific integrations out of
apps/siteand into dedicated workspace packages (@node-core/platform-vercel,@node-core/platform-cloudflare), selected at build time viaNEXT_PUBLIC_DEPLOY_TARGET(set byvercel.jsonand the OpenNextbuildCommand).Each adapter exports a
next.platform.config.mjswith a{ nextConfig, aliases, images, mdx }shape thatapps/sitemerges into its Next.js config, MDX plugins, and Playwright config via dynamic import. No-opapps/site/next.platform.config.mjsandapps/site/playwright.platform.config.mjskeep standalonepnpm dev/pnpm build/pnpm deployworking when no target is set.Removes the last platform forks from
apps/site—VERCEL_ENV,OPEN_NEXT_CLOUDFLARE,'Cloudflare' in global, andPLAYWRIGHT_RUN_CLOUDFLARE_PREVIEWare all gone.