review follow-up (#68): trim nextjs() config, move examples/scripts to root scripts/#78
Closed
wmadden-electric wants to merge 1 commit into
Closed
review follow-up (#68): trim nextjs() config, move examples/scripts to root scripts/#78wmadden-electric wants to merge 1 commit into
wmadden-electric wants to merge 1 commit into
Conversation
Addresses the PR #68 review comments: - nextjs(): `appDir` is now optional, defaulting to the authoring file's own directory ('.'). The two example storefronts kept their service in `src/`, which forced the cryptic `appDir: '..'`; move each `service.ts` up to the Next project root (beside next.config, matching datahub's own web service), so the declaration is just `nextjs({ module: import.meta.url })`. `module` stays — it's the ADR-0004 authoring-file token, identical to `node()`. - Move `examples/scripts/` (shared e2e CI tooling: ci-cleanup, cold-connect canary, destroy-guard) to root `scripts/` — it is not example apps. Fix the refs: e2e-deploy.yml, the deploy-verify-destroy action (absolute ${{ github.workspace }}/scripts paths, working-dir-independent), test:scripts, gotchas.md. - Add a unit test locking the `appDir` default; refresh stale doc comments. Green: build, typecheck (50), test (41), lint, test:scripts (111), the storefront-auth integration test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
commit: |
wmadden-electric
marked this pull request as draft
July 15, 2026 14:26
Contributor
Author
|
Superseded — the real fix is the assembler reading Next's manifest, not the authoring-side change this PR had. See the replacement PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #68 — carries the review-comment fixes that missed the #68 merge
window (auto-merge landed #68 at the conflict-resolution commit before these
were pushed).
Addresses the remaining #68 review comments:
nextjs()config trimmed.appDiris now optional, defaulting to theauthoring file's own directory (
.). The two example storefronts kept theirservice in
src/, which forced the crypticappDir: '..'; eachservice.tsmoves up to the Next project root (beside
next.config, matching datahub'sown web service), so the declaration is just
nextjs({ module: import.meta.url }).modulestays — it's the ADR-0004 authoring-file token, identical tonode().examples/scripts/→ rootscripts/. It's shared e2e CI tooling(ci-cleanup, the cold-connect canary, destroy-guard), not example apps. Refs
fixed:
e2e-deploy.yml, thedeploy-verify-destroyaction (absolute${{ github.workspace }}/scriptspaths, working-dir-independent),test:scripts,gotchas.md.appDirdefault; refreshed stale doc comments.Green locally: build, typecheck (50), test (41), lint, test:scripts (111), the
storefront-auth integration test.
🤖 Generated with Claude Code