Conversation
WalkthroughContext now tracks a resolved layouts directory, adds Changes
Sequence Diagram(s)sequenceDiagram
participant FS as File System
participant Context as Context (pages watcher)
participant Scan as scanLayouts
participant Utils as invalidateAndReload
participant Vite as ViteDevServer / ModuleGraph
Note over Context: pages.json change detected
Context->>FS: read cached previous pages.json
Context->>FS: load new pages.json
Context->>Context: diff layout fields (old vs new)
alt layouts changed for page(s)
Context->>FS: check existence of `page.vue` / `page.nvue`
Context->>Utils: call invalidateAndReload(filePath, server)
Utils->>Vite: resolve module id / find module in graph
alt module found
Utils->>Vite: invalidate module
Utils->>Vite: trigger dev-server reload
Vite-->>Utils: reload acknowledged
Utils-->>Context: return success
else module missing
Utils-->>Context: warn & return false
end
else no layout changes
Context-->>Context: no action
end
Note over Context,Scan: layout file add/unlink
FS->>Context: file add/unlink event
Context->>Scan: call reloadLayouts() -> scanLayouts(layoutDirPath)
Scan-->>Context: updated layout list (virtual module updated)
Context->>Vite: invalidate main entry modules to re-run imports
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/utils.ts (1)
97-122: Clarify whetherfilePathis a URL or filesystem path (current code assumes URL)
getModuleByUrl(filePath)implies callers must pass a Vite module URL like/src/foo.vue(not an absolute filesystem path). Naming iturl(and optionally normalizing) avoids “module not found” surprises.-// 使模块失效并重新加载 -export async function invalidateAndReload(filePath: string, server?: ViteDevServer) { +// 使模块失效并重新加载（url 应为 Vite 模块 URL，如 /src/pages/index.vue） +export function invalidateAndReload(url: string, server?: ViteDevServer) { if (!server) { - console.error('Vite server not available.') + console.error('[vite-plugin-uni-layouts] Vite server not available.') return false } - const module = await server.moduleGraph.getModuleByUrl(filePath) + const normalizedUrl = normalizePath(url) + const module = server.moduleGraph.getModuleByUrl(normalizedUrl) if (module) { server.moduleGraph.invalidateModule(module) server.reloadModule(module) return true } else { - console.warn(`[vite-plugin-uni-layouts] ❌ Module not found in module graph: ${filePath}`) + console.warn(`[vite-plugin-uni-layouts] ❌ Module not found in module graph: ${normalizedUrl}`) return false } }src/context.ts (1)
50-70: Optional: avoid callback-stylefs.access+ fire-and-forget async calls
Not a blocker, butfs.promises.access+awaitwould make ordering/error handling clearer (and makes it easier toawait invalidateAndReloadif you keep it async).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/context.ts(3 hunks)src/utils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/context.ts (1)
src/utils.ts (2)
loadPagesJson(28-46)invalidateAndReload(98-122)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/context.ts (1)
51-70: Path resolution is incorrect and will fail in many environments.This issue was flagged in a previous review and remains unresolved. The current approach has two problems:
resolve('/src', ...)on Unix resolves to an absolute system path/src/..., not a Vite-relative URLresolve('src', ...)forfs.accessresolves relative toprocess.cwd(), which may differ from Vite's rootDerive paths from
this._server.config.rootfor correctness:if (this._server && changedPages.length > 0) { + const root = this._server.config.root for (const page of changedPages) { - // 使 .vue 文件失效, 如果存在的话 - const pagePathVue = normalizePath( - resolve('/src', `${page.path}.vue`), - ) - fs.access(resolve('src', `${page.path}.vue`), fs.constants.F_OK, (err) => { - if (!err) - invalidateAndReload(pagePathVue, this._server) - }) - - // 使 .nvue 文件失效, 如果存在的话 - const pagePathNv = normalizePath( - resolve('/src', `${page.path}.nvue`), - ) - fs.access(resolve('src', `${page.path}.nvue`), fs.constants.F_OK, (err) => { - if (!err) - invalidateAndReload(pagePathNv, this._server) - }) + const candidates = [`${page.path}.vue`, `${page.path}.nvue`] + for (const ext of candidates) { + const abs = resolve(root, 'src', ext) + fs.access(abs, fs.constants.F_OK, (err) => { + if (err) + return + // Vite expects URLs relative to root with leading / + const url = normalizePath(`/src/${ext}`) + invalidateAndReload(url, this._server) + }) + } } }
🧹 Nitpick comments (2)
src/context.ts (2)
39-48: Path-based layout comparison looks correct; consider tightening the path check.The Map-based comparison by
page.pathcorrectly addresses the previous issue with index-based comparison. However,path.includes('pages.json')is a loose check that could match unrelated files likecustom-pages.json.- if (path.includes('pages.json')) { + if (path.endsWith(this.pageJsonPath) || path.endsWith('pages.json')) {
57-69: Consider usingfs.promises.accessfor cleaner async flow.The callback-based approach works but using promises would be more consistent with modern async patterns and easier to reason about, especially if error handling needs to be added later.
+import { access, constants } from 'node:fs/promises' // Then in the handler: - fs.access(resolve('src', `${page.path}.vue`), fs.constants.F_OK, (err) => { - if (!err) - invalidateAndReload(pagePathVue, this._server) - }) + try { + await access(absPath, constants.F_OK) + await invalidateAndReload(viteUrl, this._server) + } + catch { + // File doesn't exist, skip + }Note: This would require making the watcher callback
async.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/context.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/context.ts (1)
src/utils.ts (2)
loadPagesJson(28-46)invalidateAndReload(98-122)
🔇 Additional comments (1)
src/context.ts (1)
1-2: LGTM!The new imports for path resolution, filesystem access, and the
invalidateAndReloadutility are appropriate for the HMR functionality being added.Also applies to: 13-13
|
@KeJunMao 麻烦 review 下 |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/context.ts (1)
58-76: Hardcoded'/src'module URL + mixed path bases likely breaks for non-default Vite roots / non-srclayouts (and on Windows).
You check existence viaresolve(viteRoot, 'src', ...), but invalidate viaresolve('/src', ...), which can point at the wrong URL formoduleGraph.getModuleByUrl(). Also,invalidateAndReload()is async but invoked fire-and-forget from a callback—add error handling to avoid silent failures.Suggested rewrite (derive URL from
server.config.root, and handle both.vue/.nvueuniformly):- const viteRoot = this._server!.config.root - - if (!this._server || !changedPages.length) + if (!this._server || !changedPages.length) return + const viteRoot = this._server.config.root // 失效对应的模块，触发 transform for (const page of changedPages) { - // 使 .vue 文件失效, 如果存在的话 - const pagePathVue = normalizePath( - resolve('/src', `${page.path}.vue`), - ) - fs.access(resolve(viteRoot, 'src', `${page.path}.vue`), fs.constants.F_OK, (err) => { - if (!err) - invalidateAndReload(pagePathVue, this._server) - }) - - // 使 .nvue 文件失效, 如果存在的话 - const pagePathNv = normalizePath( - resolve('/src', `${page.path}.nvue`), - ) - fs.access(resolve(viteRoot, 'src', `${page.path}.nvue`), fs.constants.F_OK, (err) => { - if (!err) - invalidateAndReload(pagePathNv, this._server) - }) + for (const ext of ['.vue', '.nvue'] as const) { + const abs = resolve(viteRoot, 'src', `${page.path}${ext}`) + fs.access(abs, fs.constants.F_OK, (err) => { + if (err) + return + // Vite URLs are rooted at `/` and relative to `server.config.root` + const url = normalizePath(`/${abs.slice(normalizePath(viteRoot).length + 1)}`) + void invalidateAndReload(url, this._server).catch((e) => { + console.warn('[vite-plugin-uni-layouts] invalidateAndReload failed:', e) + }) + }) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/context.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/context.ts (1)
src/utils.ts (2)
loadPagesJson(28-46)invalidateAndReload(98-122)
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/scan.ts (1)
7-12: Test bug attest/scanLayouts.test.ts:7:scanLayouts()accepts only one parameter but test passes twoThe function signature is
scanLayouts(dir: string), but the test callsscanLayouts('src/layouts', cwd)passing a second argument that the function ignores. Update the test to pass the full resolved path as the single argument, similar to how production code does it insrc/context.ts:scanLayouts(resolve(__dirname, 'fixtures', 'src/layouts')).The production call sites are correct—
Contextproperly passesthis.layoutDirPath = resolve(options.cwd, options.layoutDir)at lines 33–34 and 103 insrc/context.ts, ensuring absolute paths are used.
♻️ Duplicate comments (1)
src/context.ts (1)
47-85: Watcher path match +_server!ordering + hardcoded/srcinvalidation URL are fragile (Windows + non-srcroots)
path.includes(this.pageJsonPath)can misfire; chokidar typically passes absolute paths—prefer normalized equality against the resolvedpages.jsonabsolute path.const viteRoot = this._server!.config.rootis read before the null-guard.resolve('/src', ...)is not a safe way to construct a Vite URL (notably on Windows it can becomeC:/src/...), and it hardcodes/src.Suggested shape (illustrative—adapt to your existing
pageJsonPath/root decisions):watcher.on('change', async (path) => { - if (!path.includes(this.pageJsonPath)) + const absPagesJson = resolve(this._server?.config.root ?? this.options.cwd, this.pageJsonPath) + if (normalizePath(path) !== normalizePath(absPagesJson)) return const prePages = this.pages this.pages = loadPagesJson(this.pageJsonPath, this.options.cwd) @@ - const viteRoot = this._server!.config.root - - if (!this._server || !changedPages.length) + if (!this._server || !changedPages.length) return + const viteRoot = this._server.config.root for (const page of changedPages) { - const pagePathVue = normalizePath(resolve('/src', `${page.path}.vue`)) - fs.access(resolve(viteRoot, 'src', `${page.path}.vue`), fs.constants.F_OK, (err) => { + const absVue = resolve(viteRoot, 'src', `${page.path}.vue`) + fs.access(absVue, fs.constants.F_OK, (err) => { if (!err) - invalidateAndReload(pagePathVue, this._server) + invalidateAndReload(normalizePath(`/${relative(viteRoot, absVue)}`), this._server) })#!/bin/bash # Verify how pages.json path is set/resolved, and how invalidateAndReload is called. rg -n --type=ts -C3 "pageJsonPath\\s*=|setupWatcher\\(|watcher\\.on\\('change'|invalidateAndReload\\(" src/context.ts # Verify virtual module resolveId/load id (is it prefixed with \\0?) rg -n --type=ts -C3 "virtualModuleId|resolveId\\s*\\(|load\\s*\\(|\\x00virtual:|\\\\0virtual:" src
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/context.ts(4 hunks)src/scan.ts(1 hunks)src/utils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/context.ts (4)
src/types.ts (3)
ResolvedOptions(20-20)Page(29-32)Layout(22-27)src/scan.ts (1)
scanLayouts(7-48)src/utils.ts (3)
loadPagesJson(28-46)invalidateAndReload(98-122)isLayoutFile(125-129)src/constant.ts (1)
virtualModuleId(1-1)
GreatAuk
changed the title
|
@FliPPeDround 麻烦 review 下 |
1 participant
Description 描述
1. 监听 pages.json 中页面 layout 变化，针对变改的页面，重新触发 transform。
看之前的机制，更新
pages.json后，vite会触发所有文件的transfrom。 其实没更新layout的页面并不需要，造成了浪费。而且更新
vite版本到 5.2.8（目前 uni app 官方模板）后，更新pages.json并不会触发所有文件的transfrom。这样导致开发过程中，你动态更新pages.json并不生效，要重新运行dev命令才能生效2. fix: 开发过程中，在layouts 目录下新增了 layout 文件，页面使用该 layout 无法生效
新增 layout 文件，无法生效，要重新运行
dev命令才能生效。而且如果删除某个 layout,vite还会报错无法找到当前文件Linked Issues 关联的 Issues
Additional context 额外上下文
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.