fix(test): package workspace deps in deploy harness#1786
Conversation
The Next.js deploy-suite harness installs vinext into isolated temp apps outside the vinext workspace. That breaks when the packaged vinext manifest still contains workspace dependencies, because pnpm cannot resolve workspace:* specs without the monorepo package graph. Copy the local @vinext/cloudflare build output into a sibling throwaway package and rewrite workspace dependencies in the generated vinext manifest to point at that package. This preserves the package boundary while keeping deploy-suite fixture installs self-contained.
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the deploy-harness change. The core approach is sound: rewriting workspace:* to a sibling file: dependency and materializing a throwaway @vinext/cloudflare package mirrors the existing .vinext-local-package pattern exactly, and the relative file:../.vinext-local-cloudflare-package path resolves correctly under pnpm (it is resolved relative to the declaring .vinext-local-package/package.json).
Things that check out:
resolveManifestDepsnow handles bothcatalog:andworkspace:specs, and routesworkspace:through the new explicit resolver.workspaceDependencySpecForthrows on any unknown workspace dependency, so a future workspace dep can't silently leak into the temp manifest. Good defensive choice.- The recursive
cpSyncofpackages/cloudflare/distpreservesdist/cache/*anddist/utils/*, which theexportsmap (./dist/cache/*) depends on. cloudflarePkg.peerDependencies(vinext: >=0.0.0) passes through unchanged, satisfied by the locally installedvinext.
One non-blocking suggestion: the harness guards ${VINEXT_PKG_DIR}/dist/cli.js near line 259 with a friendly "Build vinext first" message, but there is no equivalent guard for packages/cloudflare/dist. If cloudflare hasn't been built, the new fs.cpSync(packages/cloudflare/dist, ...) throws an ENOENT inside the node heredoc, which is harder to diagnose than the existing guard. Consider adding a parallel check next to the existing one:
if [ ! -d "${VINEXT_DIR}/packages/cloudflare/dist" ]; then
echo "@vinext/cloudflare dist not found at ${VINEXT_DIR}/packages/cloudflare/dist" >&2
echo "Build first: corepack pnpm build" >&2
exit 1
fiOverall this looks correct and well-scoped for restoring the deploy suite. LGTM with the optional guard above.
| type: cloudflarePkg.type, | ||
| files: ['dist'], | ||
| exports: cloudflarePkg.exports, | ||
| peerDependencies: resolveManifestDeps(cloudflarePkg.peerDependencies), |
There was a problem hiding this comment.
Worth noting (not a blocker — validation passed): the generated local @vinext/cloudflare keeps peerDependencies: { vinext: ">=0.0.0" }, while the local vinext manifest declares @vinext/cloudflare: file:.... That's a circular package relationship in the temp app. pnpm handles it fine here, but if a future change tightens cloudflare's peer range on vinext, the throwaway vinext version (copied verbatim from the workspace) would need to satisfy it. Just flagging for awareness.
|
Review posted successfully on PR #1786. SummaryI reviewed the deploy-harness change in What it does: Makes the Next.js deploy-suite temp app installs self-contained by (1) copying the built Verified correct:
Feedback posted (both non-blocking):
Conclusion: LGTM with the optional guard suggestion. |
2 participants
Overview
@vinext/cloudflarepackage into the temp app and rewriteworkspace:*in the generated localvinextpackage manifest to a siblingfile:dependency.scripts/e2e-deploy.shvinextpackage outside the monorepo workspace.Why
The deploy harness runs inside isolated Next.js fixture directories, not inside the vinext pnpm workspace. Any generated package manifest installed there must only contain dependency specs that the fixture package manager can resolve locally or from the registry. A raw
workspace:*dependency violates that boundary and causespnpm installto fail before the upstream test behaviour can run.What changed
vinextpackage has@vinext/cloudflare: workspace:*ERR_PNPM_WORKSPACE_PKG_NOT_FOUND..vinext-local-cloudflare-packagefrompackages/cloudflare/distand points the generatedvinextmanifest at it.workspaceDependencySpecFor()unless the harness knows how to package it.Validation
vp env exec --node 24 ./scripts/run-nextjs-deploy-suite.sh /Users/nathan/Projects/vinext/.refs/nextjs-v16.2.6 --retries 0 -c 1 --debug test/e2e/app-dir/searchparams-static-bailout/searchparams-static-bailout.test.tssearchParamsassertions, which are being handled separately.References
workspace:specs are workspace-local and are not resolvable from isolated temp fixture apps.Non-goals
This PR does not fix the
searchParamsrendering behaviour. It only restores the deploy-suite harness so those failures can be reproduced and fixed in a separate PR.