-
-
Notifications
You must be signed in to change notification settings - Fork 27
perf: Optimize file structure and correct type naming #253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughRenames PageContext to Context; replaces PageMetaDatum-centric APIs with Page/PageFile and PathSet models; updates lifecycle hook parameter types to Context; adds getPathSets and defineUniPages; restructures types and schema to PagesJson/Page; adjusts virtual module typings, exports, and tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as Tool/CLI
participant Ctx as Context
participant Files as getPathSets
participant PF as PageFile
participant Merge as mergePageMetaDataArray
participant Decl as writeDeclaration
participant VM as virtual:uni-pages
CLI->>Ctx: new Context(options)
Ctx->>Ctx: loadUserPagesConfig() → PagesJson
Note over Ctx: onBeforeLoadUserConfig / onAfterLoadUserConfig (ctx)
Ctx->>Files: getPathSets(pagesDir, options)
Files-->>Ctx: PathSet[]
Note over Ctx: onBeforeScanPages / onAfterScanPages (ctx)
loop for each PathSet
Ctx->>PF: new PageFile(ctx, path)
PF->>PF: readPageMetaFromFile / parse SFC
PF-->>Ctx: PagesJSON.Page
end
Ctx->>Merge: mergePageMetaDataArray(pages: PagesJSON.Page[])
Merge-->>Ctx: merged PagesJSON.Page[]
Note over Ctx: onBeforeMergePageMetaData / onAfterMergePageMetaData (ctx)
Ctx-->>VM: expose pages: PagesJSON.Page[], subPackages: PagesJSON.SubPackage[]
Ctx->>Decl: writeDeclaration(ctx, filepath)
Decl-->>Ctx: declaration file written
Note over Ctx: onBeforeWriteFile / onAfterWriteFile (ctx)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/index.ts (1)
34-41
: Avoid creating pages.json before configResolved (can write to the wrong location).This pre-init check uses process.cwd()/VITE_ROOT_DIR and userOptions.outDir, which may differ from resolved config.root/outDir. It can create a stray pages.json alongside the real one written later by ctx.updatePagesJSON().
Apply this diff to rely on the resolved path used by Context.updatePagesJSON():
- // TODO: check if the pages.json file is valid - const resolvedPagesJSONPath = path.join( - process.env.VITE_ROOT_DIR || process.cwd(), - userOptions.outDir ?? 'src', - OUTPUT_NAME, - ) - checkPagesJsonFileSync(resolvedPagesJSONPath) + // Defer pages.json creation to ctx.updatePagesJSON() after configResolved
🧹 Nitpick comments (6)
packages/core/src/utils.ts (1)
49-62
: Function signature updated correctly.The parameter and return types have been properly updated from
PageMetaDatum[]
toPage[]
, and the function logic is fully compatible with the newPage
interface structure.Optional: Consider renaming the function for consistency.
The function name
mergePageMetaDataArray
still references the old "PageMetaData" terminology. For complete consistency with the type rename, consider renaming tomergePageArray
:-export function mergePageMetaDataArray(pageMetaData: Page[]) { +export function mergePageArray(pageMetaData: Page[]) {This is a minor naming consistency improvement and can be addressed in a future refactor if preferred.
packages/core/src/files.ts (1)
27-40
: Normalize after computing relative; avoid slashing inputs topath.relative
.Compute abs/rel first, then
slash()
the results for consistent cross‑platform output.export function getPathSets(dir: string, options: ResolvedOptions): PathSet[] { - const pagesDirPath = slash(path.resolve(options.root, dir)) - const basePath = slash(path.join(options.root, options.outDir)) + const pagesDirPath = path.resolve(options.root, dir) + const basePath = path.join(options.root, options.outDir) const files = getPageFiles(pagesDirPath, options) debug.pages(dir, files) - const pagePaths = files - .map(file => slash(file)) - .map(file => ({ - rel: path.relative(basePath, slash(path.resolve(pagesDirPath, file))), - abs: slash(path.resolve(pagesDirPath, file)), - })) + const pagePaths = files + .map(file => path.resolve(pagesDirPath, file)) + .map(abs => ({ + rel: slash(path.relative(basePath, abs)), + abs: slash(abs), + })) return pagePaths }packages/core/src/types/uniapp/pages.ts (1)
17-21
: Considerunknown
instead ofany
for custom props.Tightens type safety while staying flexible.
- [key: string]: any + [key: string]: unknownpackages/core/src/pageFile.ts (1)
14-15
: Use a more unique global symbol key.
Symbol.for('type')
is generic and may collide. Namespace it to avoid cross‑lib conflicts.-export const PAGE_TYPE_KEY = Symbol.for('type') +export const PAGE_TYPE_KEY = Symbol.for('vite-plugin-uni-pages:type')packages/core/src/context.ts (2)
159-165
: Normalize path before comparing againstpagesConfigSourcePaths
.On Windows, backslashes may prevent
.includes
match. Normalize like other handlers.- watcher.on('change', async (path) => { - if (this.pagesConfigSourcePaths.includes(path)) { + watcher.on('change', async (path) => { + path = slash(path) + if (this.pagesConfigSourcePaths.map(slash).includes(path)) { debug.pages(`Config source changed: ${path}`) if (await this.updatePagesJSON()) this.onUpdate() } })
293-297
: Avoid mutatingpagesGlobConfig.tabBar.list
via shared array reference.Clone list to prevent side effects on loaded config.
- const tabBar = { - ...this.pagesGlobConfig?.tabBar, - list: this.pagesGlobConfig?.tabBar?.list || [], - } + const tabBar = { + ...this.pagesGlobConfig?.tabBar, + list: (this.pagesGlobConfig?.tabBar?.list || []).slice(), + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/core/README.md
(1 hunks)packages/core/client.d.ts
(1 hunks)packages/core/src/config.ts
(1 hunks)packages/core/src/config/index.ts
(0 hunks)packages/core/src/context.ts
(12 hunks)packages/core/src/declaration.ts
(2 hunks)packages/core/src/files.ts
(2 hunks)packages/core/src/index.ts
(2 hunks)packages/core/src/options.ts
(2 hunks)packages/core/src/pageFile.ts
(4 hunks)packages/core/src/types/index.ts
(1 hunks)packages/core/src/types/uniapp/index.ts
(3 hunks)packages/core/src/types/uniapp/pages.ts
(1 hunks)packages/core/src/types/uniapp/subPackages.ts
(2 hunks)packages/core/src/types/unipages.ts
(5 hunks)packages/core/src/types/utils.ts
(1 hunks)packages/core/src/utils.ts
(2 hunks)packages/schema/package.json
(1 hunks)packages/schema/schema.json
(6 hunks)packages/volar/src/schema.ts
(1 hunks)test/generate.spec.ts
(8 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/config/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T03:00:09.715Z
Learnt from: xiaohe0601
PR: uni-helper/vite-plugin-uni-pages#229
File: packages/core/client.d.ts:9-11
Timestamp: 2025-08-28T03:00:09.715Z
Learning: In uni-helper/vite-plugin-uni-pages, global type declarations like `definePage` are kept in a separate `client.d.ts` file and exposed via the `./client` export. Users need to manually add `uni-helper/vite-plugin-uni-pages/client` to their tsconfig.json to access these global declarations, which is documented as the intended usage pattern.
Applied to files:
packages/core/src/types/uniapp/index.ts
packages/core/src/types/uniapp/pages.ts
test/generate.spec.ts
packages/core/src/utils.ts
packages/core/src/config.ts
packages/core/client.d.ts
packages/core/src/types/unipages.ts
packages/core/src/index.ts
🧬 Code graph analysis (13)
packages/core/src/types/uniapp/index.ts (1)
packages/core/src/types/uniapp/pages.ts (1)
Pages
(23-23)
packages/core/src/types/uniapp/pages.ts (1)
packages/core/src/types/uniapp/globalStyle/index.ts (1)
GlobalStyle
(13-361)
packages/core/src/types/uniapp/subPackages.ts (1)
packages/core/src/types/uniapp/pages.ts (1)
Pages
(23-23)
test/generate.spec.ts (2)
packages/core/src/types/unipages.ts (1)
UserPagesJson
(156-156)packages/core/src/context.ts (1)
Context
(29-490)
packages/core/src/options.ts (1)
packages/core/src/types/uniapp/index.ts (1)
PagesJson
(25-147)
packages/core/src/utils.ts (1)
packages/core/src/types/uniapp/pages.ts (1)
Page
(3-21)
packages/core/src/config.ts (1)
packages/core/src/types/unipages.ts (1)
UserPagesJson
(156-156)
packages/core/client.d.ts (1)
packages/core/src/types/uniapp/pages.ts (1)
Page
(3-21)
packages/core/src/pageFile.ts (3)
packages/core/src/context.ts (1)
Context
(29-490)packages/core/src/types/unipages.ts (2)
PathSet
(119-122)UserPageMeta
(137-154)packages/core/src/types/uniapp/pages.ts (1)
Page
(3-21)
packages/core/src/files.ts (2)
packages/core/src/types/unipages.ts (2)
ResolvedOptions
(98-117)PathSet
(119-122)packages/core/src/utils.ts (1)
debug
(23-33)
packages/core/src/types/unipages.ts (4)
packages/core/src/types/uniapp/index.ts (1)
PagesJson
(25-147)packages/core/src/utils.ts (1)
debug
(23-33)packages/core/src/types/uniapp/pages.ts (1)
Page
(3-21)packages/core/src/types/utils.ts (1)
DeepPartial
(11-15)
packages/core/src/index.ts (2)
packages/core/src/types/unipages.ts (1)
UserOptions
(96-96)packages/core/src/context.ts (1)
Context
(29-490)
packages/core/src/context.ts (7)
packages/core/src/types/uniapp/index.ts (1)
PagesJson
(25-147)packages/core/src/pageFile.ts (2)
PageFile
(16-108)PAGE_TYPE_KEY
(14-14)packages/core/src/types/uniapp/pages.ts (1)
Page
(3-21)packages/core/src/types/uniapp/subPackages.ts (1)
SubPackage
(3-28)packages/core/src/files.ts (1)
getPathSets
(27-40)packages/core/src/types/unipages.ts (1)
PathSet
(119-122)packages/core/src/types/utils.ts (1)
KnownKeys
(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (windows-latest, 20)
🔇 Additional comments (22)
packages/core/src/utils.ts (1)
2-2
: LGTM! Import updated correctly.The import change from
PageMetaDatum
toPage
aligns with the PR's type renaming objective.packages/core/README.md (1)
117-124
: LGTM!The lifecycle hook parameter types are consistently updated from
PageContext
toContext
across all eight hooks, accurately reflecting the type rename.packages/core/src/types/uniapp/index.ts (2)
4-4
: LGTM!The addition of the
Pages
import and corresponding re-exports properly supports the type system refactor fromPageMetaDatum[]
toPages
.Also applies to: 10-10, 14-14
25-25
: LGTM!The renaming of
PagesConfig
toPagesJson
and the type change fromPageMetaDatum[]
toPages
align with the PR's objective to use more reasonable type names.Also applies to: 34-34
packages/core/src/options.ts (1)
2-2
: LGTM!The type references are correctly updated from
PagesConfig
toPagesJson
, maintaining type consistency throughout the configuration loading logic.Also applies to: 37-37
packages/core/src/types/uniapp/subPackages.ts (1)
1-1
: LGTM!The import path update and type change from
PageMetaDatum[]
toPages
align with the module reorganization and type refactoring objectives.Also applies to: 12-12
packages/core/src/config.ts (1)
1-5
: LGTM!The
defineUniPages
helper function follows the common pattern for type-safe configuration definition. This identity function provides TypeScript type checking for user config files while requiring no runtime overhead.packages/core/src/types/index.ts (1)
1-3
: LGTM!Centralizing type exports through a single index file improves maintainability and provides a clean, consolidated API surface for consumers of these types.
packages/volar/src/schema.ts (1)
3-4
: Confirm removal ofpath
requirement in Page schema
Previously, definitions.Page.required included["path"]
; clearing it removes that constraint. Ensure this aligns with intended schema flexibility.packages/core/src/index.ts (1)
17-20
: Imports/exports and ctx creation align with the refactor. LGTM.Also applies to: 27-27, 32-32, 46-46
packages/core/src/types/uniapp/pages.ts (1)
3-23
: New Page typing looks good.test/generate.spec.ts (1)
26-26
: Tests updated forContext
and new types look consistent.Also applies to: 159-162, 305-311, 343-348
packages/core/src/types/utils.ts (1)
1-15
: Utility types are well-formed and useful.packages/core/client.d.ts (1)
2-5
: Type source switch is correct; ensureDefinePage
is re-exported inpackages/core/src/index.ts
soimport { DefinePage } from '.'
works.packages/core/src/types/unipages.ts (4)
15-16
: ConfigSource typing looks good.Using
LoadConfigSource<PagesJson>
aligns withloadConfig<PagesJson>
usage downstream.
86-93
: Lifecycle hooks typing aligns with Context.Callbacks accept
Context
and match usages incontext.ts
.
119-122
: PathSet shape matches usage.Consistent with getPathSets return and PageFile constructor.
137-143
: User types exposed cleanly.
- UserPageMeta = Partial + optional
type
is clear.- UserPagesJson = DeepPartial is ergonomic.
- definePage signature stays compatible with macros.
Also applies to: 156-156, 158-160
packages/schema/schema.json (3)
2-2
: Root schema referencingPagesJson
is correct.Keeps the schema anchored to the public PagesJson definition.
2683-2705
: Page/Pages/PagesJson linkage is coherent.
- Page allows custom properties via
additionalProperties
.- Pages is an array of Page.
- PagesJson.pages correctly references Pages.
Also applies to: 2706-2711, 2712-2712, 2736-2738
2831-2833
: Use only “provider” and “version” in subPackages[].plugins; remove “export”
Per uni-app pages.json spec, subpackage plugin entries support only provider (required) and version (required); export is not supported in subPackages.packages/core/src/context.ts (1)
199-220
: Overall: parsing/merging logic aligns with new types; platform-aware merge is solid.
- PageFile-based pipeline and platform merge via comment-json are well structured.
- SubPackages merging respects roots and normalizes relative page paths.
- Virtual module outputs and declaration generation are consistent.
Consider adding unit tests for:
- Home page selection and ordering.
- Platform-specific pages merging (ifdef comments preserved).
- SubPackages path normalization.
Also applies to: 250-255, 257-278, 420-458, 492-648
} | ||
|
||
result.sort(page => (page.type === 'home' ? -1 : 0)) | ||
result.sort(page => ((page as any)[PAGE_TYPE_KEY] === 'home' ? -1 : 0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broken Array.sort comparator; home page ordering may be incorrect.
Comparator takes only one arg; must compare a and b.
- result.sort(page => ((page as any)[PAGE_TYPE_KEY] === 'home' ? -1 : 0))
+ result.sort((a, b) =>
+ ((a as any)[PAGE_TYPE_KEY] === 'home' ? -1 : ((b as any)[PAGE_TYPE_KEY] === 'home' ? 1 : 0)),
+ )
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
result.sort(page => ((page as any)[PAGE_TYPE_KEY] === 'home' ? -1 : 0)) | |
result.sort((a, b) => | |
((a as any)[PAGE_TYPE_KEY] === 'home' | |
? -1 | |
: ((b as any)[PAGE_TYPE_KEY] === 'home' ? 1 : 0))), | |
) |
🤖 Prompt for AI Agents
In packages/core/src/context.ts around line 245, the Array.sort comparator is
incorrect because it only takes one argument; replace it with a proper
two-argument comparator like result.sort((a, b) => { const aIsHome = (a as
any)[PAGE_TYPE_KEY] === 'home'; const bIsHome = (b as any)[PAGE_TYPE_KEY] ===
'home'; if (aIsHome && !bIsHome) return -1; if (!aIsHome && bIsHome) return 1;
return 0; }) so items with page type 'home' are ordered before others while
preserving relative order for ties.
export function getDeclaration(ctx: Context) { | ||
const subPagesPath = ctx.subPackages.map((sub) => { | ||
return sub.pages.map(v => (`"/${normalizePath(join(sub.root, v.path))}"`)) | ||
}).flat() | ||
const tabsPagesPath = ctx.pagesGlobConfig?.tabBar?.list?.map((v) => { | ||
return `"/${v!.pagePath}"` | ||
}) ?? [] | ||
const allPagesPath = [...ctx.pageMetaData.filter(page => !tabsPagesPath.includes(page.path)).map(v => `"/${v.path}"`), ...subPagesPath] | ||
const allPagesPath = [...ctx.pages.filter(page => !tabsPagesPath.includes(page.path)).map(v => `"/${v.path}"`), ...subPagesPath] | ||
const code = `/* eslint-disable */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TabBar pages exclusion check is broken (format mismatch).
tabsPagesPath items are like "/pages/index"
but you compare against page.path
(no leading slash), so tab pages aren’t excluded.
- const allPagesPath = [...ctx.pages.filter(page => !tabsPagesPath.includes(page.path)).map(v => `"/${v.path}"`), ...subPagesPath]
+ const allPagesPath = [
+ ...ctx.pages
+ .filter(page => !tabsPagesPath.includes(`"/${normalizePath(page.path)}"`))
+ .map(v => `"/${normalizePath(v.path)}"`),
+ ...subPagesPath,
+ ]
🤖 Prompt for AI Agents
In packages/core/src/declaration.ts around lines 8 to 16, the exclusion check
fails because tabsPagesPath entries are created as quoted, leading-slash strings
(e.g. '"/pages/index"') while page.path has no leading slash (e.g.
'pages/index'); update the construction so both formats match: remove the
surrounding quotes and the leading slash when building tabsPagesPath (or
alternately add a leading slash to page.path before comparing) so the contains
check works; ensure tabsPagesPath and the values used in the includes(...) call
use the same canonical form (no extra quotes, consistent leading slash
presence).
import type { PageContext } from './context' | ||
import type { debug } from './utils' | ||
import type { Context } from '../context' | ||
import type { debug } from '../utils' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type-only import breaks keyof typeof debug
(TS compile error).
import type { debug }
cannot be used with typeof debug
. Use typeof import()
or a value import. Minimal fix:
-import type { debug } from '../utils'
+// use inline typeof import to avoid a runtime import
- debug: boolean | keyof typeof debug
+ debug: boolean | keyof typeof import('../utils').debug
Also applies to: 84-84
🤖 Prompt for AI Agents
In packages/core/src/types/unipages.ts around lines 4 and 84, the current
"import type { debug }" is causing a TS error when you use "keyof typeof debug"
because type-only imports cannot be used with typeof; replace the type-only
import with a value import (import { debug } from '../utils') or change the type
usage to use the import() form (e.g. keyof typeof import('../utils').debug) so
the compiler can evaluate the runtime typeof; update both occurrences
accordingly.
方不方便做这个 #191 |
类型应该更新到 |
我认为可能不太合适,
或者可以叫 |
uni-helper 的各个库的命名都太累赘了,而且还拆分得太细。 比如类型,其实出一个 然后在
同时也可以做个映射: import * as t from '@uni-helper/types';
t.pagesJson.XXX // pages.json 的类型 当然,也可以放在 uni-pages 里,将类型挂载到 然后在 |
我认为你说得很有道理,从语义和结构清晰度来看, 但之所以项目是目前的命名方式,主要是考虑到 npm 和 GitHub 上大多数用户的搜索习惯。很多开发者可能并不熟悉 很多时候包名的选择并不完全取决于语义是否最佳,而是历史沿用和社区习惯的结果,这也是为了降低大多数用户的使用门槛。 当然,这只是一个权衡的问题,没有绝对的对错。只要我们能在文档中能清楚地说明这些就好。 |
我推荐使用 |
如果是独立包名,那么 |
在 PR 描述里新增了注意事项。 |
注意:此PR为破坏性更新。 将属于 uniapp 的 pages.json 的官方类型汇集到 PagesJSON 里,并规范化 uni-pages 的一些类型名称。
理应这些类型变更用户是无感的,因为一般使用上无须从 uni-pages 导入类型。但由于 uni-pages 一直都是全部 export 出去,不确定是否有用户有使用这些类型。
我的观点是:一个优秀的库必然存在抛弃旧包袱,不断进化的过程。而不是一直在旧的问题上盖屎山。
Summary by CodeRabbit
Breaking Changes
New Features
Documentation