ReviewOS
Reload

also looking at this

stacks/stx

feat(layouts): honour a fallback layouts directory

#1942
Merged chris wants to merge fix/fallback-layouts-dir into main
5 files +174 -10

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

packages/bun-plugin/src/serve.tsmodified+13-0
@@ -537,6 +537,15 @@ export interface ServeOptions {
537537 openPath?: string
538538 componentsDir?: string
539539 layoutsDir?: string
540 /**
541 * Layouts to fall back to when `layoutsDir` does not have the one a page
542 * asks for, so a framework can ship defaults an app overrides by name.
543 *
544 * Stacks has been passing this since the views server was written; until
545 * stx honoured it, it did nothing, and every page extending a framework
546 * layout served a 200 with an empty body.
547 */
548 fallbackLayoutsDir?: string
540549 partialsDir?: string
541550 /**
542551 * Additional source directories that can affect rendered HTML.
@@ -1015,6 +1024,7 @@ export async function serve(options: ServeOptions): Promise<void> {
10151024 // Options passed directly take precedence, then bunfig config, then defaults
10161025 const componentsDir = options.componentsDir ?? stxConfig.componentsDir ?? defaultStxConfig.componentsDir
10171026 const layoutsDir = options.layoutsDir ?? stxConfig.layoutsDir ?? defaultStxConfig.layoutsDir
1027 const fallbackLayoutsDir = options.fallbackLayoutsDir ?? stxConfig.fallbackLayoutsDir
10181028 const partialsDir = options.partialsDir ?? stxConfig.partialsDir ?? defaultStxConfig.partialsDir
10191029 const publicDir = options.publicDir ?? stxConfig.publicDir ?? 'public'
10201030
@@ -2203,6 +2213,7 @@ function __stxOverlay(errs){
22032213 root: process.cwd(),
22042214 ...(componentsDir && { componentsDir }),
22052215 ...(layoutsDir && { layoutsDir }),
2216 ...(fallbackLayoutsDir && { fallbackLayoutsDir }),
22062217 ...(partialsDir && { partialsDir }),
22072218 autoShell: true,
22082219 buildMode: 'serve' as const,
@@ -2576,6 +2587,7 @@ function __stxOverlay(errs){
25762587 root: process.cwd(),
25772588 ...(componentsDir && { componentsDir }),
25782589 ...(layoutsDir && { layoutsDir }),
2590 ...(fallbackLayoutsDir && { fallbackLayoutsDir }),
25792591 ...(partialsDir && { partialsDir }),
25802592 autoShell: true,
25812593 buildMode: 'serve' as const,
@@ -3164,6 +3176,7 @@ function __stxOverlay(errs){
31643176 ...defaultConfig,
31653177 ...(componentsDir && { componentsDir }),
31663178 ...(layoutsDir && { layoutsDir }),
3179 ...(fallbackLayoutsDir && { fallbackLayoutsDir }),
31673180 ...(partialsDir && { partialsDir }),
31683181 autoShell: false,
31693182 }
packages/stx/src/process.tsmodified+19-9
@@ -1084,20 +1084,30 @@ async function processDirectivesInternal(
10841084 // compare against, the message was easy to scroll past — which is how a
10851085 // page ends up asserting a layout group with no layout behind it
10861086 // (#1879).
1087 let available: string[] = []
1088 try {
1089 const dir = resolvedOptions.layoutsDir
1090 if (dir) {
1087 //
1088 // Both directories are listed, not just `layoutsDir`. Reporting only
1089 // the app's own layouts made a resolvable-but-unconfigured fallback
1090 // look like the layout did not exist anywhere, which sent the search
1091 // in the wrong direction entirely.
1092 const layoutsIn = (dir: string | undefined): string[] => {
1093 if (!dir)
1094 return []
1095 try {
10911096 const abs = path.isAbsolute(dir) ? dir : path.resolve(process.cwd(), dir)
1092 available = fs.readdirSync(abs)
1097 return fs.readdirSync(abs)
10931098 .filter(f => f.endsWith('.stx'))
10941099 .map(f => f.replace(/\.stx$/, ''))
1095 .sort()
1100 }
1101 catch {
1102 // Unreadable directory — the message is still worth emitting.
1103 return []
10961104 }
10971105 }
1098 catch {
1099 // Unreadable layoutsDir — the message is still worth emitting.
1100 }
1106
1107 const available = [
1108 ...layoutsIn(resolvedOptions.layoutsDir),
1109 ...layoutsIn(resolvedOptions.fallbackLayoutsDir),
1110 ].filter((name, index, all) => all.indexOf(name) === index).sort()
11011111
11021112 const warning = `Layout not found: ${layoutPath} (referenced from ${filePath})`
11031113 + (available.length > 0
packages/stx/src/types/config-types.tsmodified+11-0
@@ -687,6 +687,17 @@ export interface StxConfig {
687687 _rootComponentsDir?: string
688688 /** Path to layouts directory, defaults to 'layouts' in the project root */
689689 layoutsDir?: string
690 /**
691 * Where to look for a layout `layoutsDir` does not have.
692 *
693 * For a framework that ships default layouts and lets an app override them
694 * by name: the app's own directory is `layoutsDir` and wins, and anything it
695 * does not define falls back here. Without it, an app that overrides one
696 * layout has to vendor every other layout it uses, and a page extending a
697 * framework layout renders with NO layout at all - a 200 with an empty body,
698 * which is far harder to diagnose than a 404.
699 */
700 fallbackLayoutsDir?: string
690701 /**
691702 * Path to the stores directory, resolved against `root`.
692703 *
packages/stx/src/utils.tsmodified+31-1
@@ -1492,7 +1492,7 @@ export async function resolveTemplatePath(
14921492 const safeUnderCwd = assertInsideRoot(result, process.cwd())
14931493 if (safeUnderCwd) return safeUnderCwd
14941494
1495 const trustedDirs = [options.root, options.layoutsDir, options.componentsDir, options.partialsDir, options.pagesDir]
1495 const trustedDirs = [options.root, options.layoutsDir, options.fallbackLayoutsDir, options.componentsDir, options.partialsDir, options.pagesDir]
14961496 .filter((d): d is string => typeof d === 'string' && d.length > 0)
14971497 for (const dir of trustedDirs) {
14981498 const resolvedDir = path.isAbsolute(dir) ? dir : path.resolve(process.cwd(), dir)
@@ -1655,6 +1655,36 @@ async function resolveTemplatePathInner(
16551655 }
16561656 }
16571657
1658 // The app's layouts directory did not have it, so try the one the framework
1659 // ships. This is the override model the caller is expressing: `layoutsDir`
1660 // is the app and wins, `fallbackLayoutsDir` is the default set behind it.
1661 //
1662 // Checked AFTER `options.layoutsDir` on purpose - an app that defines a
1663 // layout of the same name must win, which is the whole point of shipping
1664 // defaults. Missing this step is not a 404 but a 200 with an empty body,
1665 // because a page whose layout does not resolve renders its sections into
1666 // nothing.
1667 if (options.fallbackLayoutsDir) {
1668 const resolvedFallback = path.isAbsolute(options.fallbackLayoutsDir)
1669 ? options.fallbackLayoutsDir
1670 : path.resolve(process.cwd(), options.fallbackLayoutsDir)
1671
1672 const strippedPath = templatePath.startsWith('layouts/') ? templatePath.slice(8) : templatePath
1673 const candidates = [path.join(resolvedFallback, strippedPath)]
1674 if (!templatePath.endsWith('.stx'))
1675 candidates.push(`${candidates[0]}.stx`)
1676
1677 for (const candidate of candidates) {
1678 if (await fileExists(candidate)) {
1679 if (options.debug)
1680 console.log(`Found in options.fallbackLayoutsDir: ${candidate}`)
1681 if (dependencies)
1682 dependencies.add(candidate)
1683 return candidate
1684 }
1685 }
1686 }
1687
16581688 // 4. Special case for layouts directory if specified in options or path looks like 'layouts/*'
16591689 if (templatePath.startsWith('layouts/') || templatePath.includes('/layouts/')) {
16601690 const parts = templatePath.split('layouts/')
packages/stx/test/fallback-layouts.test.tsadded+100-0
@@ -0,0 +1,100 @@
1// A framework ships default layouts; an app overrides the ones it cares about.
2//
3// Without a fallback, an app that defines one layout has to vendor every other
4// layout it uses, and a page extending a framework layout renders with NO
5// layout at all. That failure is a 200 with an empty body - the sections have
6// nowhere to render into - which is far harder to diagnose than a 404, and is
7// exactly how a whole staff dashboard shipped blank to production.
8
9import { describe, expect, test } from 'bun:test'
10import { mkdirSync, mkdtempSync, writeFileSync } from 'node:fs'
11import { tmpdir } from 'node:os'
12import { join } from 'node:path'
13import { resolveTemplatePath } from '../src/utils'
14
15function tree(files: Record<string, string>): string {
16 const root = mkdtempSync(join(tmpdir(), 'stx-layouts-'))
17 for (const [rel, body] of Object.entries(files)) {
18 const full = join(root, rel)
19 mkdirSync(join(full, '..'), { recursive: true })
20 writeFileSync(full, body)
21 }
22 return root
23}
24
25describe('fallbackLayoutsDir', () => {
26 test('finds a layout the app does not define', async () => {
27 const root = tree({
28 'app/layouts/marketing.stx': '<html></html>',
29 'defaults/layouts/default.stx': '<html>framework</html>',
30 'app/views/page.stx': '@extends(\'layouts/default\')',
31 })
32
33 const resolved = await resolveTemplatePath('layouts/default', join(root, 'app/views/page.stx'), {
34 layoutsDir: join(root, 'app/layouts'),
35 fallbackLayoutsDir: join(root, 'defaults/layouts'),
36 } as never)
37
38 expect(resolved).toBe(join(root, 'defaults/layouts/default.stx'))
39 })
40
41 test('the app still wins for a layout it does define', async () => {
42 // The point of shipping defaults is that they can be overridden. If the
43 // fallback won, an app could never replace a framework layout.
44 const root = tree({
45 'app/layouts/default.stx': '<html>app</html>',
46 'defaults/layouts/default.stx': '<html>framework</html>',
47 'app/views/page.stx': '@extends(\'layouts/default\')',
48 })
49
50 const resolved = await resolveTemplatePath('layouts/default', join(root, 'app/views/page.stx'), {
51 layoutsDir: join(root, 'app/layouts'),
52 fallbackLayoutsDir: join(root, 'defaults/layouts'),
53 } as never)
54
55 expect(resolved).toBe(join(root, 'app/layouts/default.stx'))
56 })
57
58 test('resolves a nested layout name', async () => {
59 const root = tree({
60 'app/layouts/marketing.stx': '<html></html>',
61 'defaults/layouts/dashboard/default.stx': '<html>dashboard</html>',
62 'app/views/page.stx': '@extends(\'layouts/dashboard/default\')',
63 })
64
65 const resolved = await resolveTemplatePath('layouts/dashboard/default', join(root, 'app/views/page.stx'), {
66 layoutsDir: join(root, 'app/layouts'),
67 fallbackLayoutsDir: join(root, 'defaults/layouts'),
68 } as never)
69
70 expect(resolved).toBe(join(root, 'defaults/layouts/dashboard/default.stx'))
71 })
72
73 test('stays null when neither directory has it', async () => {
74 const root = tree({
75 'app/layouts/marketing.stx': '<html></html>',
76 'defaults/layouts/default.stx': '<html></html>',
77 'app/views/page.stx': '@extends(\'layouts/nope\')',
78 })
79
80 const resolved = await resolveTemplatePath('layouts/nope', join(root, 'app/views/page.stx'), {
81 layoutsDir: join(root, 'app/layouts'),
82 fallbackLayoutsDir: join(root, 'defaults/layouts'),
83 } as never)
84
85 expect(resolved).toBeNull()
86 })
87
88 test('changes nothing when no fallback is configured', async () => {
89 const root = tree({
90 'app/layouts/marketing.stx': '<html></html>',
91 'app/views/page.stx': '@extends(\'layouts/default\')',
92 })
93
94 const resolved = await resolveTemplatePath('layouts/default', join(root, 'app/views/page.stx'), {
95 layoutsDir: join(root, 'app/layouts'),
96 } as never)
97
98 expect(resolved).toBeNull()
99 })
100})