fix vitest test error #225
Conversation
WalkthroughAdds a local AST toolkit under packages/core/src/ast-kit (parsers, type guards, language helpers, parse cache), swaps imports to the local toolkit, replaces ast-kit with @babel/parser in package.json, changes execScript to accept an imports array and updates VM transpilation/execution behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant ASTKit as Local AST Kit
participant Cache as parseCache
participant Babel as @babel/parser
Caller->>ASTKit: babelParse(code, lang?, { cache?, ...opts })
ASTKit->>ASTKit: getBabelParserOptions(lang, opts)
note right of ASTKit #DDEBF7: enable plugins (typescript/dts, jsx,\ndecorators, import assertions) based on lang
alt cache enabled
ASTKit->>Cache: lookup(code)
alt cache hit
Cache-->>ASTKit: cached ParseResult
ASTKit-->>Caller: Program + metadata
else cache miss
ASTKit->>Babel: parse(code, options)
Babel-->>ASTKit: ParseResult<t.File>
ASTKit->>Cache: store(code -> result)
ASTKit-->>Caller: Program + ParseResult
end
else cache disabled
ASTKit->>Babel: parse(code, options)
Babel-->>ASTKit: ParseResult<t.File>
ASTKit-->>Caller: Program + ParseResult
end
sequenceDiagram
autonumber
actor Caller
participant Page as page.ts
participant Utils as execScript
participant VM as vm.Context
Page->>Utils: execScript(imports[], code, filename)
Utils->>Utils: tmpCode = imports.join('\n') + '\nexport default ' + code
Utils->>Utils: transpile tmpCode -> jsCode (CommonJS)
Utils->>VM: runInNewContext(jsCode, context, { timeout: 1000 })
alt execution success
VM-->>Utils: exported value (default or exports)
Utils->>Caller: invoke if function and return result
else error
VM-->>Utils: throw
Utils-->>Caller: throw Error("EXEC SCRIPT FAIL IN filename: ...")
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
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/utils.ts (1)
86-91: createRequire should be initialized with the file path, not the directory.Passing the directory can break resolution semantics. Use the absolute filename.
- require: createRequire(dir), + require: createRequire(filename), module: {}, exports: {},
🧹 Nitpick comments (7)
packages/core/package.json (1)
61-61: ✅ @babel/parser v7.28.3 supports the required pluginsThe pinned
@babel/parserversion (^7.28.3) already ships with the following parser‐plugin identifiers enabled:
- explicitResourceManagement (introduced in v7.20.0) (babeljs.io)
- importAttributes (introduced in v7.22.0) (babeljs.io)
- deprecatedImportAssert (legacy import-attributes syntax) (babeljs.io)
No bump of
@babel/parseris necessary to enable these plugins.
Optional refactor: align all other@babel/*packages (e.g.@babel/types,@babel/generator) to the same minor version (^7.28.3) to avoid subtle cross-package incompatibilities.packages/core/src/utils.ts (2)
74-80: transpileModule options: remove ineffective flags and fix misleading comments.
- noEmit has no effect with transpileModule and doesn’t control type-checking here.
- strict affects type checking, which transpileModule does not perform.
- jsDocParsingMode is not a TranspileOptions field; if accepted by your TS version, it’s incidental. Prefer to omit it.
- The “Node.js 10+ compatibility” comment is misleading given Vite 5 requires Node 18+. Targeting ES2020 (or higher) is a better default.
Apply this diff:
module: ts.ModuleKind.CommonJS, - target: ts.ScriptTarget.ES2018, // 提高兼容性, 兼容 Node.js 10+ - noEmit: true, // 不进行类型检查 - strict: false, // 禁用严格模式 - removeComments: true, // 移除注释 + // Vite 5 requires Node.js 18+, so ES2020+ is safe and avoids unnecessary downleveling. + target: ts.ScriptTarget.ES2020, + removeComments: true, // 移除注释 }, - jsDocParsingMode: ts.JSDocParsingMode.ParseNone, // 不解析 JSDocIf you truly need to disable JSDoc parsing for perf, we can discuss a custom fast path (e.g., stripping with a lightweight prepass).
93-101: Improve execution safety and diagnostics for vm.Script.
- Attach filename for better stack traces.
- Consider a small timeout to avoid accidental infinite loops from user code.
- Ensure module.exports and exports stay in sync for CommonJS semantics.
- const script = new vm.Script(jsCode) + const script = new vm.Script(jsCode, { filename }) try { - script.runInNewContext(vmContext) + // Keep module.exports and exports linked + // (in case transpiled code mutates either side) + // Note: keep vmContext.exports as the single source of truth. + // @ts-expect-error augment at runtime + vmContext.module = { exports: vmContext.exports } + script.runInNewContext(vmContext, { timeout: 1000 })Optionally, we can whitelist require to built-ins only to reduce risk.
Happy to wire a minimal “safeRequire” that blocks filesystem/network by default.
packages/core/src/ast-kit/lang.ts (1)
12-16: getLang: minor nit—avoid double work by trimming the query before extname.Functionally fine as-is. If you’d like, trim the query once to avoid allocating extra strings.
-export function getLang(filename: string): string { - if (isDts(filename)) - return 'dts' - return extname(filename).replace(/^\./, '').replace(/\?.*$/, '') -} +export function getLang(filename: string): string { + if (isDts(filename)) + return 'dts' + const base = filename.split('?')[0]! + return extname(base).replace(/^\./, '') +}packages/core/src/ast-kit/check.ts (1)
3-12: isCallOf only matches direct identifiers; document or extend if member calls are expected.Current guard returns false for calls like foo.bar(). If the callers ever need to match those (e.g., router.definePage()), consider extending to support MemberExpression patterns.
I can add an overload that accepts "foo.bar" strings and matches MemberExpression(callee: Identifier('foo'), property: Identifier('bar')).
packages/core/src/ast-kit/parse.ts (1)
49-78: Parser options: ensure plugin set is minimal and stable.
- Pushing both importAttributes and deprecatedImportAssert is fine for compatibility, but consider ordering and future deprecation. When importAttributes is present, importAssertions/deprecatedImportAssert is typically unnecessary unless parsing legacy code.
- decorators-legacy + typescript is fine; just be aware of decorator metadata proposals if you ever switch to the “decorators” plugin.
No changes required now—flagging for awareness.
If you want, I can add a small “normalizePlugins” helper to dedupe and order plugins deterministically.
packages/core/src/page.ts (1)
10-11: Unify imports via the barrel to reduce path coupling.Since
./ast-kit/index.tsre-exportscheck, prefer importing bothbabelParseandisCallOffrom the barrel. This avoids deep import paths that can become brittle if the folder structure changes.Apply this diff:
-import { babelParse } from './ast-kit' -import { isCallOf } from './ast-kit/check' +import { babelParse, isCallOf } from './ast-kit'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
packages/core/package.json(1 hunks)packages/core/src/ast-kit/check.ts(1 hunks)packages/core/src/ast-kit/index.ts(1 hunks)packages/core/src/ast-kit/lang.ts(1 hunks)packages/core/src/ast-kit/parse.ts(1 hunks)packages/core/src/index.ts(1 hunks)packages/core/src/page.ts(1 hunks)packages/core/src/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/core/src/ast-kit/index.ts (1)
packages/core/src/declaration.ts (1)
v(12-14)
packages/core/src/ast-kit/parse.ts (1)
packages/core/src/ast-kit/lang.ts (2)
isTs(32-34)REGEX_LANG_JSX(5-5)
⏰ 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 (4)
packages/core/package.json (1)
1-87: No external “ast-kit” imports remain; only local “./ast-kit” references foundAll scans for
import … from 'ast-kit'andrequire('ast-kit')returned no matches. The only occurrences are intentional local imports (import … from './ast-kit') in:
- packages/core/src/page.ts
- packages/core/src/index.ts
No further action needed.
packages/core/src/index.ts (1)
11-11: Good migration to local AST utilities.Switching
babelParseto the local./ast-kitbarrel aligns with the PR goal of removing the externalast-kit. No functional differences expected if the local API mirrors the old one.packages/core/src/page.ts (1)
102-104: Script to verify the pinned@babel/parserversion has been generated. Returning results will confirm whether the configured parser supportsimportAttributesanddeprecatedAssertSyntax.packages/core/src/ast-kit/index.ts (1)
1-3: Barrel looks good and keeps AST helpers co-located.Re-exporting
check,lang, andparseprovides a clean single entry point. This also enables nicer imports elsewhere (see suggestion inpage.ts).
| export function isIdentifier( | ||
| node?: t.Node | undefined | null, | ||
| ): node is t.Identifier { | ||
| return !!node && (node.type === 'Identifier' || node.type === 'JSXIdentifier') | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type guard is unsound: returns true for JSXIdentifier but narrows to t.Identifier.
This can mislead callers into treating a JSXIdentifier as an Identifier. Either drop JSXIdentifier or widen the predicate’s return type.
-export function isIdentifier(
- node?: t.Node | undefined | null,
-): node is t.Identifier {
- return !!node && (node.type === 'Identifier' || node.type === 'JSXIdentifier')
-}
+export function isIdentifier(
+ node?: t.Node | null | undefined,
+): node is t.Identifier {
+ return !!node && node.type === 'Identifier'
+}If you truly need JSX identifiers, add a dedicated helper:
export function isJsxIdentifier(node?: t.Node | null): node is t.JSXIdentifier {
return !!node && node.type === 'JSXIdentifier'
}🤖 Prompt for AI Agents
In packages/core/src/ast-kit/check.ts around lines 32-36, the current
isIdentifier type guard returns true for JSXIdentifier but types it as
t.Identifier, which is unsound. Fix by removing the JSXIdentifier branch so
isIdentifier only checks for node.type === 'Identifier' and add a new helper
isJsxIdentifier that checks node.type === 'JSXIdentifier'; update exports and
any call sites to use the appropriate guard (or, alternatively, if you prefer
one guard to cover both, change the return type to node is t.Identifier |
t.JSXIdentifier and update usages accordingly).
| export function babelParse( | ||
| code: string, | ||
| lang?: string, | ||
| { cache, ...options }: ParserOptions & { cache?: boolean } = {}, | ||
| ): t.Program & Omit<ParseResult<t.File>, 'type' | 'program'> { | ||
| let result: ParseResult<t.File> | undefined | ||
| if (cache) | ||
| result = parseCache.get(code) | ||
|
|
||
| if (!result) { | ||
| result = parse(code, getBabelParserOptions(lang, options)) | ||
| if (cache) | ||
| parseCache.set(code, result) | ||
| } | ||
|
|
||
| const { program, type, ...rest } = result | ||
| return { ...program, ...rest } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Cache key ignores lang and parser options—can yield incorrect ASTs.
Two different parse contexts sharing identical code (e.g., ts vs tsx, JSX on/off) will collide. Include lang and relevant options in the cache key.
-export function babelParse(
+export function babelParse(
code: string,
lang?: string,
{ cache, ...options }: ParserOptions & { cache?: boolean } = {},
): t.Program & Omit<ParseResult<t.File>, 'type' | 'program'> {
let result: ParseResult<t.File> | undefined
if (cache)
- result = parseCache.get(code)
+ result = parseCache.get(makeCacheKey(code, lang, options))
if (!result) {
result = parse(code, getBabelParserOptions(lang, options))
if (cache)
- parseCache.set(code, result)
+ parseCache.set(makeCacheKey(code, lang, options), result)
}Add this helper (outside the selected range):
function makeCacheKey(code: string, lang?: string, options: ParserOptions = {}) {
// Only include option fields that affect parsing semantics.
const plugins = (options.plugins || []).map(p => Array.isArray(p) ? p[0] : p).sort();
return JSON.stringify({ code, lang: lang || '', plugins });
}🤖 Prompt for AI Agents
In packages/core/src/ast-kit/parse.ts around lines 20 to 37, the cache currently
keys only on the source code which causes collisions for different languages or
parser options; add a cache key helper that includes lang and the relevant
parsing-affecting options (e.g., normalized plugins list) and use it when
reading/writing parseCache so different parse contexts (ts vs tsx, JSX on/off,
plugin differences) do not collide; implement a makeCacheKey(code, lang,
options) that serializes code, lang (or empty string) and a sorted, normalized
plugins array (extracting plugin names when arrays are used) and replace
parseCache.get(code)/set(code, result) with parseCache.get(makeCacheKey(...)) /
parseCache.set(makeCacheKey(...), result).
| import { bold, dim, lightYellow, link } from 'kolorist' | ||
| import MagicString from 'magic-string' | ||
| import { createLogger } from 'vite' | ||
| import { babelParse } from './ast-kit' |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify API parity and parser option handling in the new babelParse.
This file calls babelParse with 2 params (code, lang), while page.ts also calls it with an extra parser options object. Please confirm the local babelParse:
- Accepts both the 2-arg and 3-arg forms.
- Merges user-provided parser options (e.g., additional plugins) instead of overwriting defaults.
- Keys the parse cache (if any) by content + lang + parser options to avoid cross-pollution of ASTs.
Also verify that @babel/parser in packages/core/package.json supports the importAttributes syntax option used in page.ts. If not, bump it.
Run this script to check export signatures, cache usage, and remaining external imports:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check local babelParse export/signature =="
rg -nC2 -P 'export\s+(function|const)\s+babelParse|\bexport\s*{\s*babelParse\s*}' packages/core/src/ast-kit || true
rg -nC2 -P 'function\s+babelParse\s*\(|const\s+babelParse\s*[:=]' packages/core/src/ast-kit/parse.ts || true
echo "== Inspect parse cache/keying (content/lang/options) =="
rg -nC3 -P 'parseCache|new Map|cache' packages/core/src/ast-kit/parse.ts || true
echo "== Ensure @babel/parser is present and sufficiently new =="
fd -a package.json | while read f; do
echo "--- $f"
jq -r '.dependencies["@babel/parser"], .devDependencies["@babel/parser"]' "$f" 2>/dev/null
done
echo "== Find any lingering external 'ast-kit' imports =="
rg -nP 'from\s+[\"\']ast-kit(?:/check)?[\"\']' -g '!**/dist/**' || trueLength of output: 3662
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate getBabelParserOptions =="
rg -nC5 -P 'function\s+getBabelParserOptions' -g '!**/dist/**' packages/core/src/ast-kit
echo "== ParserOptions definition =="
rg -nC5 -P 'export\s+(type|interface)\s+ParserOptions' -g '!**/dist/**' packages/core/src/ast-kit
echo "== Inspect merging behavior in getBabelParserOptions =="
rg -nC5 -P 'getBabelParserOptions.*\{\s*.*\.\.\.options' -g '!**/dist/**' packages/core/src/ast-kitLength of output: 1194
🌐 Web query:
Does @babel/parser version ^7.28.3 support the importAttributes option?
Error during web search
Action Required: Refine babelParse Option Handling and Cache Keying
-
babelParse Signature
• Supports both two- and three-argument forms viaexport function babelParse( code: string, lang?: string, { cache, ...options }: ParserOptions & { cache?: boolean } = {}, )
(so calling
babelParse(code, lang)andbabelParse(code, lang, opts)both compile) -
Parser Options Merging (packages/core/src/ast-kit/parse.ts)
•getBabelParserOptions(lang, options)seedspluginsfromoptions.pluginsand unconditionally doesreturn { sourceType: 'module', ranges: true, tokens: true, plugins, ...options, }
• However, spreading
...optionslast will overwrite the mergedpluginsarray wheneveroptions.pluginsis provided—dropping the injectedtypescript/jsxdefaults.
→ Fix: Move...optionsbefore thepluginskey or excludepluginsfrom the final spread so defaults and user-provided plugins are always merged. -
Parse Cache Keying
• Currently usesparseCache.get(code)andset(code, result), ignoringlangand other parser options.
→ Fix: Includelangand a stable serialization ofoptionsin the cache key (e.g.key = [code, lang, JSON.stringify(options)].join('|')) to prevent incorrect AST reuse across different parse contexts. -
@babel/parser Version Compatibility
• packages/core@… depends on@babel/parser^7.28.3, which — per the Babel 7.21.0 changelog — supports theimportAttributessyntax option used inpage.ts.
→ Action: Add a unit/integration test incorethat exercisesimportAttributesparsing under page-like scenarios to catch any regressions. No immediate version bump required.
Please apply the above fixes to ensure robust defaults merging, correct cache isolation, and sustained support for import-assertion syntax.
🤖 Prompt for AI Agents
In packages/core/src/index.ts around line 11 and related files
(packages/core/src/ast-kit/parse.ts), the babelParse export should accept both
two- and three-argument forms (code, lang?, opts?) where opts may include
cache:boolean; update the babelParse signature/dispatch to accept an optional
third ParserOptions object and a cache flag. In parse.ts fix options merging so
user-provided plugins do not overwrite injected defaults: either spread
...options before specifying plugins or omit plugins from the final ...options
spread and build plugins by concatenating defaults + (options.plugins || []).
Change the parse cache key to include lang and a stable serialization of options
(e.g. join [code, lang, JSON.stringify(options)]) so different langs/options
don’t reuse the same AST. Finally, add a unit/integration test in core that
parses code using the importAttributes/import assertions syntax to guard against
regressions with @babel/parser.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/core/src/page.ts(2 hunks)packages/core/src/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/page.ts
⏰ 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). (2)
- GitHub Check: test (windows-latest, 18)
- GitHub Check: test (ubuntu-latest, 18)
🔇 Additional comments (1)
packages/core/src/utils.ts (1)
71-80: Transpilation options validated—no changes required
- ts.transpileModule skips a full type-checking pass, so the
strictflag (which only affects type checking) does not alter its output (typescriptlang.org).- Nullish coalescing (
??) and optional chaining are automatically down-leveled by TypeScript when targeting ES2018, ensuring compatibility with older runtimes (stackoverflow.com).No modifications to the
compilerOptionsin this call tots.transpileModuleare necessary.
packages/core/src/utils.ts
Outdated
| // 创建一个新的虚拟机上下文，支持动态导入 | ||
| const vmContext = { | ||
| require: createRequire(dir), | ||
| module: {}, | ||
| module: { | ||
| _compile: (code: string, _filename: string) => { | ||
| return new vm.Script(code).runInThisContext() | ||
| }, | ||
| }, | ||
| exports: {}, | ||
| __filename: filename, | ||
| __dirname: dir, | ||
| require: createRequire(dir), | ||
| // 提供一个 import 函数用于动态导入 | ||
| import: (id: string) => import(id), | ||
| } |
There was a problem hiding this comment.
Fix module._compile implementation and improve vmContext setup.
The current module._compile implementation doesn't properly handle the compiled code and may cause issues. Additionally, the vmContext setup could be more robust.
- // 创建一个新的虚拟机上下文，支持动态导入
- const vmContext = {
- module: {
- _compile: (code: string, _filename: string) => {
- return new vm.Script(code).runInThisContext()
- },
- },
- exports: {},
- __filename: filename,
- __dirname: dir,
- require: createRequire(dir),
- // 提供一个 import 函数用于动态导入
- import: (id: string) => import(id),
- }
+ // 创建一个新的虚拟机上下文，支持动态导入
+ const vmContext = {
+ console,
+ Buffer,
+ process: {
+ env: process.env,
+ cwd: process.cwd,
+ },
+ __filename: filename,
+ __dirname: dir,
+ require: createRequire(dir),
+ // 提供一个 import 函数用于动态导入
+ import: async (id: string) => {
+ // Handle relative imports properly
+ if (id.startsWith('.')) {
+ const resolvedPath = path.resolve(dir, id)
+ return import(resolvedPath)
+ }
+ return import(id)
+ },
+ }📝 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.
| // 创建一个新的虚拟机上下文，支持动态导入 | |
| const vmContext = { | |
| require: createRequire(dir), | |
| module: {}, | |
| module: { | |
| _compile: (code: string, _filename: string) => { | |
| return new vm.Script(code).runInThisContext() | |
| }, | |
| }, | |
| exports: {}, | |
| __filename: filename, | |
| __dirname: dir, | |
| require: createRequire(dir), | |
| // 提供一个 import 函数用于动态导入 | |
| import: (id: string) => import(id), | |
| } | |
| // 创建一个新的虚拟机上下文，支持动态导入 | |
| const vmContext = { | |
| console, | |
| Buffer, | |
| process: { | |
| env: process.env, | |
| cwd: process.cwd, | |
| }, | |
| __filename: filename, | |
| __dirname: dir, | |
| require: createRequire(dir), | |
| // 提供一个 import 函数用于动态导入 | |
| import: async (id: string) => { | |
| // Handle relative imports properly | |
| if (id.startsWith('.')) { | |
| const resolvedPath = path.resolve(dir, id) | |
| return import(resolvedPath) | |
| } | |
| return import(id) | |
| }, | |
| } |
🤖 Prompt for AI Agents
In packages/core/src/utils.ts around lines 84 to 97, the module._compile
currently returns a Script runInThisContext call and doesn't correctly populate
module.exports or run the code inside a proper VM context; replace it by
creating a vm.Context from vm.createContext(vmContext) and implement
module._compile to compile the code with new vm.Script(code, { filename }), run
it with script.runInContext(context), ensure the context's module and exports
are used (assigning module.exports or exports as appropriate) and return
module.exports; also initialize vmContext via vm.createContext so require,
__filename, __dirname and a safe dynamic import helper are available in the
sandbox and ensure import resolves via the host dynamic import (or a safe
wrapper) rather than leaking the parent context.
packages/core/src/utils.ts
Outdated
| try { | ||
| script.runInNewContext(vmContext) | ||
| const script = new vm.Script(wrappedCode, { | ||
| filename, | ||
| }) | ||
|
|
||
| const result = await script.runInNewContext(vmContext, { | ||
| timeout: 1000, // 设置超时避免长时间运行 | ||
| }) | ||
|
|
||
| // 如果导出的是函数，执行它 | ||
| if (typeof result === 'function') { | ||
| return Promise.resolve(result()) | ||
| } | ||
|
|
||
| return Promise.resolve(result) | ||
| } | ||
| catch (error: any) { | ||
| throw new Error(`${filename}: ${error.message}`) | ||
| } | ||
|
|
||
| // 获取导出的值 | ||
| const result = (vmContext.exports as any).default || vmContext.exports | ||
|
|
||
| // 如果是函数，执行函数并返回其返回值 | ||
| if (typeof result === 'function') { | ||
| return Promise.resolve(result()) | ||
| } | ||
|
|
||
| // 如果不是函数，返回结果 | ||
| return Promise.resolve(result) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance execution safety and error reporting.
The script execution could be improved with better error handling and safety measures:
- The timeout is hardcoded and might be insufficient for complex scripts
- Error messages could be more descriptive
- No cleanup of the VM context after execution
try {
- const script = new vm.Script(wrappedCode, {
- filename,
- })
-
- const result = await script.runInNewContext(vmContext, {
- timeout: 1000, // 设置超时避免长时间运行
- })
-
- // 如果导出的是函数，执行它
- if (typeof result === 'function') {
- return Promise.resolve(result())
- }
-
- return Promise.resolve(result)
+ const script = new vm.Script(wrappedCode, {
+ filename,
+ lineOffset: 0,
+ columnOffset: 0,
+ })
+
+ const result = await script.runInNewContext(vmContext, {
+ timeout: 5000, // Increased timeout for complex scripts
+ breakOnSigint: true,
+ })
+
+ // 如果导出的是函数，执行它
+ if (typeof result === 'function') {
+ try {
+ const functionResult = await Promise.resolve(result())
+ return functionResult
+ } catch (funcError: any) {
+ throw new Error(`${filename}: Function execution failed: ${funcError.message}`)
+ }
+ }
+
+ return result
}
+ catch (vmError: any) {
+ if (vmError.message.includes('timeout')) {
+ throw new Error(`${filename}: Script execution timeout (exceeded 5000ms)`)
+ }
+ throw new Error(`${filename}: ${vmError.message}`)
+ }
catch (error: any) {
- throw new Error(`${filename}: ${error.message}`)
+ throw new Error(`${filename}: Unexpected error: ${error.message}`)
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/core/src/utils.ts around lines 113-132, make script execution safer
by (1) replacing the hardcoded timeout with a configurable option (e.g., accept
a timeout parameter with a sensible default or read from config/env) and pass
that into runInNewContext; (2) improve the catch to rethrow a more descriptive
Error that includes the original error name, message and stack (e.g.,
`${filename}: ${error.name}: ${error.message}\n${error.stack}`) so callers get
full context; and (3) ensure VM context cleanup by running a finally block that
explicitly clears or dereferences any properties on vmContext or resets the
context variable so no references remain after execution.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/core/src/utils.ts (2)
69-75: Make timeout configurable (and increase default).The 1000ms default is tight for non-trivial macros. Add an options bag with timeoutMs and use it in runInContext.
-export async function execScript(imports: string[], code: string, filename: string): Promise<any> { +export async function execScript( + imports: string[], + code: string, + filename: string, + options: { timeoutMs?: number } = {}, +): Promise<any> { try { + const timeoutMs = options.timeoutMs ?? 5000 @@ - await script.runInNewContext(vmContext, { - timeout: 1000, // 设置超时避免长时间运行 - }) + await script.runInContext(context, { + timeout: timeoutMs, + })Also applies to: 99-101
114-116: Preserve error name/stack for easier debugging.Current message drops the original error details; rethrow with name and stack.
- catch (error: any) { - throw new Error(`EXEC SCRIPT FAIL IN ${filename}: ${error.message}`) - } + catch (error: any) { + const name = error?.name ?? 'Error' + const msg = error?.message ?? String(error) + const stack = error?.stack ? `\n${error.stack}` : '' + throw new Error(`EXEC SCRIPT FAIL IN ${filename}: ${name}: ${msg}${stack}`) + }
🧹 Nitpick comments (3)
packages/core/src/utils.ts (3)
71-73: Make export assembly robust to complex expressions/objects.Using “export default ${code}” can mis-parse for certain expressions. Assign to a temp and export that to avoid ASI/precedence pitfalls.
- const tmpCode = `${imports.join('\n')}\nexport default ${code}` + const tmpCode = `${imports.join('\n')}\nconst __uni_pages_default = (${code});\nexport default __uni_pages_default;`
74-83: Correct the comment and include fileName in transpile options.
- Keeping ModuleKind.CommonJS is intentional here; vm.Script cannot evaluate ESM with top-level import/export.
- Provide fileName to improve diagnostics and sourcemap associations (even if source maps are not emitted).
- noEmit has no effect for transpileModule; safe to drop.
- // 编译 TypeScript 代码为 JavaScript + // 编译 TypeScript 代码为 JavaScript（保留 CommonJS，vm.Script 不支持顶层 ESM） const jsCode = ts.transpileModule(tmpCode, { + fileName: filename, compilerOptions: { - module: ts.ModuleKind.CommonJS, // 改为 ESNext 以支持动态导入 + module: ts.ModuleKind.CommonJS, target: ts.ScriptTarget.ES2018, - noEmit: true, strict: false, removeComments: true, }, jsDocParsingMode: ts.JSDocParsingMode.ParseNone, // 不解析 JSDoc }).outputText
106-113: Drop unnecessary Promise.resolve noise.execScript is already async. Returning Promise.resolve is redundant and slightly obscures control flow.
- if (typeof result === 'function') { - return Promise.resolve(result()) - } - - // 如果不是函数，返回结果 - return Promise.resolve(result) + if (typeof result === 'function') + return await result() + + // 如果不是函数，返回结果 + return result
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/core/src/page.ts(2 hunks)packages/core/src/utils.ts(1 hunks)packages/playground/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/page.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/playground/tsconfig.json (3)
packages/core/src/index.ts (4)
VitePluginUniPages(48-157)transform(87-144)v(65-65)configResolved(62-85)packages/playground/vite.config.ts (1)
UserConfig(7-9)packages/core/src/config/index.ts (1)
defineUniPages(5-7)
| "paths": { | ||
| "@uni-helper/vite-plugin-uni-pages": ["./packages/core/src/index.ts"] | ||
| }, |
There was a problem hiding this comment.
Add compilerOptions.baseUrl so paths mapping actually takes effect.
TypeScript ignores compilerOptions.paths without a baseUrl. Add baseUrl at the same level as paths (under compilerOptions).
Additional change outside the selected lines:
{
"compilerOptions": {
"baseUrl": ".",
"paths": {
"@uni-helper/vite-plugin-uni-pages": ["../core/src/index.ts"]
}
}
}🤖 Prompt for AI Agents
In packages/playground/tsconfig.json around lines 9 to 11, TypeScript path
mappings won't work because compilerOptions.baseUrl is missing; add "baseUrl":
"." at the same compilerOptions level and adjust the path target to the correct
relative location (use "../core/src/index.ts" instead of
"./packages/core/src/index.ts") so the "@uni-helper/vite-plugin-uni-pages"
mapping resolves correctly.
🛠️ Refactor suggestion
Fix paths alias target (relative path is incorrect from packages/playground).
From packages/playground, "./packages/core/..." points to a non-existent nested folder. It should reference the sibling package "../core/...". Update the mapping:
- "paths": {
- "@uni-helper/vite-plugin-uni-pages": ["./packages/core/src/index.ts"]
- },
+ "paths": {
+ "@uni-helper/vite-plugin-uni-pages": ["../core/src/index.ts"]
+ },📝 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.
| "paths": { | |
| "@uni-helper/vite-plugin-uni-pages": ["./packages/core/src/index.ts"] | |
| }, | |
| "paths": { | |
| "@uni-helper/vite-plugin-uni-pages": ["../core/src/index.ts"] | |
| }, |
🤖 Prompt for AI Agents
In packages/playground/tsconfig.json around lines 9 to 11, the "paths" mapping
points to "./packages/core/src/index.ts" which is incorrect relative to
packages/playground; update the alias target to reference the sibling package
using "../core/src/index.ts" so the path resolves correctly from
packages/playground.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/core/src/utils.ts (1)
95-113: Wire module.exports correctly, expose common globals, and harden require.
- exports should alias module.exports to preserve CJS semantics.
- Expose console, Buffer, and a minimal process object to avoid ReferenceError in sandboxed code.
- Restrict require to an allowlist derived from imports; still allow relative paths. Also prefer createRequire(filename) for correct resolution.
Apply:
- // 创建一个新的虚拟机上下文，支持动态导入 - const vmContext = { - module: {}, - exports: {}, - __filename: filename, - __dirname: dir, - require: (() => { - return (id: string) => { - if (process.env.VITEST && id === '@uni-helper/vite-plugin-uni-pages') { - // eslint-disable-next-line ts/no-require-imports - return require(path.resolve(__dirname, '..')) - } - const requireFunc = createRequire(dir) - return requireFunc(id) - } - })(), - import: (id: string) => import(id), - } + // Derive an allowlist from import specifiers; relative paths always allowed + const allowed = new Set<string>( + imports + .map((l) => { + const m = l.match(/\bfrom\s+['"]([^'"]+)['"]/) + return m ? m[1] : '' + }) + .filter(Boolean), + ) + const baseRequire = createRequire(filename) + const safeRequire = (id: string) => { + if (process.env.VITEST && id === '@uni-helper/vite-plugin-uni-pages') { + // eslint-disable-next-line ts/no-require-imports + return require(path.resolve(__dirname, '..')) + } + if (id.startsWith('.') || id.startsWith('/')) return baseRequire(id) + if (allowed.has(id)) return baseRequire(id) + throw new Error(`Disallowed require("${id}") in ${filename}`) + } + // CJS semantics: exports aliases module.exports + const moduleObj = { exports: {} as any } + // 创建一个新的虚拟机上下文，支持动态导入 + const vmContext = { + console, + Buffer, + process: { + env: process.env, + cwd: process.cwd, + }, + module: moduleObj, + exports: moduleObj.exports, + __filename: filename, + __dirname: dir, + require: safeRequire, + import: async (id: string) => { + if (id.startsWith('.')) + return import(path.resolve(dir, id)) + return import(id) + }, + }If you prefer minimal change only for resolution correctness, at least adjust createRequire:
- const requireFunc = createRequire(dir) + const requireFunc = createRequire(filename)
🧹 Nitpick comments (5)
packages/core/src/context.ts (1)
279-279: Avoid eager JSON.stringify() in debug logs to prevent unnecessary overheadAs written, JSON serialization happens on every call—even when the
debugnamespace is disabled—wasting CPU/allocations on large projects. Prefer lettingdebugformat objects (only when enabled) or guard with.enabled.Preferred change (lets debug format lazily and preserves pretty struct visibility via
%O):- debug.subPages(JSON.stringify(this.subPageMetaData, null, 2)) + debug.subPages('%O', this.subPageMetaData)Alternative (if you must keep exact JSON output):
- debug.subPages(JSON.stringify(this.subPageMetaData, null, 2)) + if (debug.subPages.enabled) + debug.subPages(JSON.stringify(this.subPageMetaData, null, 2))Follow-up: Consider applying the same pattern to the similar log in
scanSubPages(Line 115) for consistency and perf parity.packages/core/src/utils.ts (4)
73-76: Reduce Vitest log noise.The banner floods test output. Use a concise debug logger or remove it.
Apply:
- if (process.env.VITEST) { - // eslint-disable-next-line no-console - console.log('!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! VITEST !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!') - } + if (process.env.VITEST) { + // eslint-disable-next-line no-console + console.debug('[execScript] Vitest mode enabled') + }
115-121: Use a contextified sandbox and make timeout configurable.create a vm.Context and run in it; expose a higher timeout or configurable one to reduce flakes in CI/Vitest.
Apply:
- // 使用 vm 模块执行 JavaScript 代码 - const script = new vm.Script(jsCode, { filename }) - - await script.runInNewContext(vmContext, { - timeout: 1000, // 设置超时避免长时间运行 - }) + // 使用 vm 模块执行 JavaScript 代码 + const script = new vm.Script(jsCode, { filename }) + const context = vm.createContext(vmContext) + const timeout = + Number(process.env.VITE_PLUGIN_UNI_PAGES_VM_TIMEOUT ?? '') || 5000 + await script.runInContext(context, { + timeout, // 避免长时间运行 + breakOnSigint: true, + })
122-131: Default export retrieval: avoid falsy traps and await function results.Use the existing getDefaultExport helper and ensure function results are awaited in this async context.
Apply:
- // 获取导出的值 - const result = (vmContext.exports as any).default || vmContext.exports + // 获取导出的值 + const result = getDefaultExport((vmContext as any).module?.exports ?? vmContext.exports) @@ - if (typeof result === 'function') { - return Promise.resolve(result()) - } + if (typeof result === 'function') { + return await Promise.resolve(result()) + } @@ - return Promise.resolve(result) + return resultIf you adopt the moduleObj change above, use moduleObj.exports in place of (vmContext as any).module?.exports.
133-135: Improve error reporting; avoid dumping generated code by default.Include the original stack for debuggability; gate jsCode output behind VITEST or DEBUG to prevent noisy logs.
Apply:
- catch (error: any) { - throw new Error(`EXEC SCRIPT FAIL IN ${filename}: ${error.message} \n\n${jsCode}\n\n`) - } + catch (error: any) { + const base = `EXEC SCRIPT FAIL IN ${filename}: ${error?.name || 'Error'}: ${error?.message || String(error)}` + const stack = error?.stack ? `\n${error.stack}` : '' + const maybeCode = process.env.VITEST || process.env.DEBUG ? `\n\n${jsCode}\n` : '' + throw new Error(base + stack + maybeCode) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/core/src/context.ts(2 hunks)packages/core/src/utils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/core/src/utils.ts (2)
packages/core/src/index.ts (1)
transform(87-144)packages/core/src/page.ts (1)
imp(106-106)
packages/core/src/context.ts (1)
test/generate.spec.ts (2)
ctx(366-394)ctx(324-364)
🔇 Additional comments (2)
packages/core/src/context.ts (1)
15-15: LGTM: import of OUTPUT_NAME is correct and used
OUTPUT_NAMEis consumed below (Line 64) to constructresolvedPagesJSONPath. No behavior change and resolves cleanly.packages/core/src/utils.ts (1)
70-76: execScript call sites verified – all usages updated
All invocations ofexecScripthave been confirmed to use the new three-argument signature. A repository-wide search shows:
- packages/core/src/page.ts:112 –
await execScript(imports, code, sfc.filename)No two-argument calls were found.
| jsCode = ts.transpileModule(tmpCode, { | ||
| compilerOptions: { | ||
| module: ts.ModuleKind.CommonJS, | ||
| target: ts.ScriptTarget.ES2018, | ||
| noEmit: true, | ||
| strict: false, | ||
| removeComments: true, | ||
| }, | ||
| jsDocParsingMode: ts.JSDocParsingMode.ParseNone, // 不解析 JSDoc | ||
| }).outputText |
There was a problem hiding this comment.
Invalid transpile option and minor config tweaks.
- jsDocParsingMode is not a valid property of ts.transpileModule options and will fail type-checking.
- noEmit is ignored by transpileModule.
- Provide fileName for better stack traces and diagnostics.
Apply:
- jsCode = ts.transpileModule(tmpCode, {
- compilerOptions: {
+ jsCode = ts.transpileModule(tmpCode, {
+ fileName: filename,
+ compilerOptions: {
module: ts.ModuleKind.CommonJS,
target: ts.ScriptTarget.ES2018,
- noEmit: true,
strict: false,
removeComments: true,
},
- jsDocParsingMode: ts.JSDocParsingMode.ParseNone, // 不解析 JSDoc
}).outputText📝 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.
| jsCode = ts.transpileModule(tmpCode, { | |
| compilerOptions: { | |
| module: ts.ModuleKind.CommonJS, | |
| target: ts.ScriptTarget.ES2018, | |
| noEmit: true, | |
| strict: false, | |
| removeComments: true, | |
| }, | |
| jsDocParsingMode: ts.JSDocParsingMode.ParseNone, // 不解析 JSDoc | |
| }).outputText | |
| jsCode = ts.transpileModule(tmpCode, { | |
| fileName: filename, | |
| compilerOptions: { | |
| module: ts.ModuleKind.CommonJS, | |
| target: ts.ScriptTarget.ES2018, | |
| strict: false, | |
| removeComments: true, | |
| }, | |
| }).outputText |
🤖 Prompt for AI Agents
In packages/core/src/utils.ts around lines 81 to 90, the call to
ts.transpileModule uses an invalid option jsDocParsingMode, includes the ignored
noEmit flag, and lacks a fileName which hurts diagnostics; remove the
jsDocParsingMode property, remove noEmit from compilerOptions, and add a
fileName string (e.g. the source path or a generated name) to the
transpileModule options so stack traces and diagnostics include the source file;
keep the existing compilerOptions (module and target) and optionally set
reportDiagnostics: true if you need transpile diagnostics.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/core/src/utils.ts (2)
90-111: Optional: tighten the sandbox and make dynamic import resolve relative paths correctly.
- The current import: (id) => import(id) will not resolve relative specifiers from the executed file’s directory. Resolve via pathToFileURL.
- Consider restricting require to an allowlist derived from imports to avoid arbitrary host access when evaluating untrusted code.
Apply for dynamic import resolution:
- import: (id: string) => import(id), + import: async (id: string) => { + if (id.startsWith('.')) { + const url = pathToFileURL(path.resolve(dir, id)).href + return import(url) + } + return import(id) + },If you want an allowlist for bare requires (optional):
- const baseRequire = createRequire(filename) + const baseRequire = createRequire(filename) + const allowed = new Set<string>( + imports + .map((l) => { + const m = l.match(/\bfrom\s+["']([^"']+)["']/) + return m ? m[1] : '' + }) + .filter(Boolean) as string[], + ) return (id: string) => { if (process.env.VITEST && id === '@uni-helper/vite-plugin-uni-pages') { const localPath = path.resolve(moduleDir, '..') // eslint-disable-next-line no-console console.log(`REPLACE @uni-helper/vite-plugin-uni-pages WITH PATH: ${localPath}`) return baseRequire(localPath) } - return baseRequire(id) + if (id.startsWith('.') || id.startsWith('/')) return baseRequire(id) + if (allowed.has(id)) return baseRequire(id) + throw new Error(`Disallowed require("${id}") in ${filename}`) }
76-85: Fix invalid/ineffective TypeScript transpile options and add fileName for better diagnostics.
- jsDocParsingMode is not a valid option for transpileModule.
- noEmit is ignored by transpileModule and should be removed.
- fileName helps produce clearer stack traces and diagnostics.
Apply:
- jsCode = ts.transpileModule(tmpCode, { - compilerOptions: { + jsCode = ts.transpileModule(tmpCode, { + fileName: filename, + compilerOptions: { module: ts.ModuleKind.CommonJS, target: ts.ScriptTarget.ES2018, - noEmit: true, strict: false, removeComments: true, }, - jsDocParsingMode: ts.JSDocParsingMode.ParseNone, // 不解析 JSDoc }).outputText
🧹 Nitpick comments (2)
packages/core/src/utils.ts (2)
113-119: Use vm.createContext + runInContext and enable breakOnSigint for safer execution.
runInNewContext takes a plain object and contextifies it implicitly; using a real Context makes sandboxing clearer and lets you enable breakOnSigint.Apply:
- const script = new vm.Script(jsCode, { filename }) - - await script.runInNewContext(vmContext, { - timeout: 1000, // 设置超时避免长时间运行 - }) + const script = new vm.Script(jsCode, { filename }) + const context = vm.createContext(vmContext) + await script.runInContext(context, { + timeout: 1000, // 设置超时避免长时间运行 + breakOnSigint: true, + })
131-133: Improve error reporting; include stack and avoid always dumping transpiled code.
Include error name/stack for debugging and gate the code dump to Vitest runs to reduce noisy logs.Apply:
- catch (error: any) { - throw new Error(`EXEC SCRIPT FAIL IN ${filename}: ${error.message} \n\n${jsCode}\n\n`) - } + catch (error: any) { + const details = process?.env?.VITEST ? `\n\nTranspiled code:\n${jsCode}\n` : '' + const name = error?.name || 'Error' + const message = error?.message || String(error) + const stack = error?.stack ? `\n${error.stack}` : '' + throw new Error(`EXEC SCRIPT FAIL IN ${filename}: ${name}: ${message}${stack}${details}`) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/core/src/utils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/src/utils.ts (1)
packages/core/src/page.ts (1)
imp(106-106)
🔇 Additional comments (2)
packages/core/src/utils.ts (2)
5-5: LGTM: importing process for Vitest-specific branching is reasonable.
Using node:process keeps it tree-shakeable and explicit.
70-70: All execScript invocations updated – no action neededI’ve checked for any 2-argument calls to execScript and found none; every invocation (e.g. packages/core/src/page.ts:112) already supplies the new imports array. No broken call sites remain.
| const vmContext = { | ||
| module: {}, | ||
| exports: {}, | ||
| __filename: filename, | ||
| __dirname: dir, | ||
| require: (() => { | ||
| return (id: string) => { | ||
| if (process.env.VITEST && id === '@uni-helper/vite-plugin-uni-pages') { | ||
| const localPath = path.resolve(__dirname, '..') | ||
| // eslint-disable-next-line no-console | ||
| console.log(`REPLACE @uni-helper/vite-plugin-uni-pages WITH PATH: ${localPath}`) | ||
| // eslint-disable-next-line ts/no-require-imports | ||
| return require(localPath) | ||
| } | ||
|
|
||
| // 如果是函数，执行函数并返回其返回值 | ||
| if (typeof result === 'function') { | ||
| return Promise.resolve(result()) | ||
| } | ||
| const requireFunc = createRequire(dir) | ||
|
|
||
| return requireFunc(id) | ||
| } | ||
| })(), | ||
| import: (id: string) => import(id), | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix ESM-incompatible require usage and align exports semantics with CommonJS.
- Calling bare require(...) inside an ESM module branch will throw (require is undefined). Use createRequire-derived function instead.
- module and exports should be wired so exports aliases module.exports (Node CJS semantics). This also makes result extraction robust.
Apply:
- // 创建一个新的虚拟机上下文，支持动态导入
- const vmContext = {
- module: {},
- exports: {},
+ // 创建一个新的虚拟机上下文，支持动态导入
+ const moduleObj = { exports: {} as any }
+ const vmContext = {
+ module: moduleObj,
+ exports: moduleObj.exports,
__filename: filename,
__dirname: dir,
- require: (() => {
- return (id: string) => {
+ require: (() => {
+ const baseRequire = createRequire(filename)
+ return (id: string) => {
if (process.env.VITEST && id === '@uni-helper/vite-plugin-uni-pages') {
- const localPath = path.resolve(__dirname, '..')
+ const localPath = path.resolve(moduleDir, '..')
// eslint-disable-next-line no-console
console.log(`REPLACE @uni-helper/vite-plugin-uni-pages WITH PATH: ${localPath}`)
- // eslint-disable-next-line ts/no-require-imports
- return require(localPath)
+ return baseRequire(localPath)
}
-
- const requireFunc = createRequire(dir)
-
- return requireFunc(id)
+ return baseRequire(id)
}
})(),
import: (id: string) => import(id),
}Outside this hunk, define moduleDir once for ESM/CJS safety:
// add near other imports
import { fileURLToPath, pathToFileURL } from 'node:url'
// add near top-level
const moduleDir = typeof __dirname !== 'undefined'
? __dirname
: path.dirname(fileURLToPath(import.meta.url))🤖 Prompt for AI Agents
In packages/core/src/utils.ts around lines 90 to 111, the VM context currently
calls bare require(...) (which will fail in ESM) and doesn't wire module/exports
to reflect CommonJS semantics; define moduleDir once (using
fileURLToPath+path.dirname when __dirname is undefined) and create a require
function via createRequire(moduleDir) to use for all require calls (including
the local plugin path) instead of calling bare require; also make module and
exports in the vmContext reference the same object (i.e., set module.exports =
exports) so exported values are retrieved consistently under CJS semantics.
| // 获取导出的值 | ||
| const result = (vmContext.exports as any).default || vmContext.exports | ||
|
|
||
| // 如果是函数，执行函数并返回其返回值 | ||
| if (typeof result === 'function') { | ||
| return Promise.resolve(result()) | ||
| } | ||
|
|
||
| // 如果不是函数，返回结果 | ||
| return Promise.resolve(result) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Await function exports and avoid Promise.resolve to ensure uniform error handling.
Returning Promise.resolve(...) inside an async function can bypass the surrounding try/catch for async rejections. Awaiting preserves the intended error wrapping.
Apply:
- const result = (vmContext.exports as any).default || vmContext.exports
+ const result = (moduleObj.exports as any).default ?? moduleObj.exports
@@
- if (typeof result === 'function') {
- return Promise.resolve(result())
- }
+ if (typeof result === 'function') {
+ return await result()
+ }
@@
- return Promise.resolve(result)
+ return resultCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/core/src/utils.ts around lines 120 to 130, the current code returns
Promise.resolve(...) which can bypass surrounding try/catch for async
rejections; change the function branch so that if result is a function you call
and await it (await result()), and for non-functions return await result (await
result) instead of Promise.resolve, ensuring all possible thrown/rejected errors
propagate into the existing async error handling.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
packages/core/src/utils.ts (6)
88-96: Optional: restrict require to an allowlist derived from imports.This reduces accidental filesystem/process access during evaluation. Safe wrapper example (derive specifiers from imports lines):
- const baseRequire = createRequire(filename) - const vmContext = { + const specifiers = new Set( + imports + .map(l => (l.match(/\bfrom\s+['"]([^'"]+)['"]/)?.[1]) || '') + .filter(Boolean), + ) + const baseRequire = createRequire(filename) + const safeRequire = (id: string) => { + if (id.startsWith('.') || id.startsWith('/')) return baseRequire(id) + if (specifiers.has(id)) return baseRequire(id) + throw new Error(`Disallowed require("${id}") in ${filename}`) + } + const vmContext = { module: moduleObj, exports: moduleObj.exports, __filename: filename, __dirname: dir, - require: baseRequire, + require: safeRequire, console, import: (id: string) => import(id), }
101-103: Increase timeout and enable breakOnSigint (or make it configurable).1000ms is tight for non-trivial transforms; breakOnSigint improves cancelability during dev.
- await script.runInNewContext(vmContext, { - timeout: 1000, // 设置超时避免长时间运行 - }) + await script.runInNewContext(vmContext, { + timeout: 5000, // 放宽超时以避免误杀 + breakOnSigint: true, + })
105-114: Await results to preserve error wrapping; prefer moduleObj.exports and nullish coalescing.Returning Promise.resolve(...) can bypass the try/catch for async rejections, losing your custom error context. Awaiting keeps errors wrapped. Also use the wired moduleObj.exports and ??.
- // 获取导出的值 - const result = (vmContext.exports as any).default || vmContext.exports + // 获取导出的值 + const result = (moduleObj.exports as any).default ?? moduleObj.exports @@ - if (typeof result === 'function') { - return Promise.resolve(result()) - } + if (typeof result === 'function') { + return await result() + } @@ - return Promise.resolve(result) + return await result
95-96: Note: global property import does not enable dynamic import syntax.Providing import: (id) => import(id) does not affect the import(...) operator. Proper support requires vm.Module/SourceTextModule or supplying importModuleDynamically. Consider removing this property or switching to a vm.Module-based path if dynamic imports are needed.
75-84: Fix ts.transpileModule options: remove invalid jsDocParsingMode, add fileName, drop noEmit.jsDocParsingMode isn’t a valid TranspileOptions property; noEmit is ignored by transpileModule; missing fileName hurts diagnostics/stack traces. This will fail type-checking.
Apply:
- jsCode = ts.transpileModule(tmpCode, { - compilerOptions: { - module: ts.ModuleKind.CommonJS, - target: ts.ScriptTarget.ESNext, - noEmit: true, - strict: false, - removeComments: true, - }, - jsDocParsingMode: ts.JSDocParsingMode.ParseNone, // 不解析 JSDoc - }).outputText + jsCode = ts.transpileModule(tmpCode, { + fileName: filename, + reportDiagnostics: true, + compilerOptions: { + module: ts.ModuleKind.CommonJS, + target: ts.ScriptTarget.ESNext, + strict: false, + removeComments: true, + }, + }).outputText
88-96: Use createRequire(filename) and wire module/exports to match CommonJS semantics.Passing a directory to createRequire is incorrect and can resolve modules unexpectedly. Also, exports should alias module.exports so either assignment style works. Provide console to avoid ReferenceError in evaluated code.
Apply:
- // 创建一个新的虚拟机上下文，支持动态导入 - const vmContext = { - module: {}, - exports: {}, - __filename: filename, - __dirname: dir, - require: createRequire(dir), - import: (id: string) => import(id), - } + // 创建一个新的虚拟机上下文，支持动态导入 + const moduleObj = { exports: {} as any } + const baseRequire = createRequire(filename) + const vmContext = { + module: moduleObj, + exports: moduleObj.exports, + __filename: filename, + __dirname: dir, + require: baseRequire, + console, + import: (id: string) => import(id), + }
🧹 Nitpick comments (9)
packages/core/tsup.config.ts (8)
4-6: Make build-mode detection resilient (prefer tsup's watch flag over NODE_ENV).Relying on process.env.NODE_ENV requires your scripts to always set it. Using tsup’s options.watch makes local dev vs. prod behavior deterministic regardless of environment setup. Also lets you drop the process import entirely.
Apply this diff:
-import process from 'node:process' -import { defineConfig } from 'tsup' +import { defineConfig } from 'tsup' -export default defineConfig(() => { - const isDev = process.env.NODE_ENV === 'development' +export default defineConfig((options) => { + const isDev = Boolean(options.watch)
13-13: Align target with supported Node runtimes (Node 18/20+) instead of ES level.Targeting es2017 forces unnecessary downleveling for modern Node. Targeting node18 (or node20 if/when you drop Node 18) keeps output leaner and closer to runtime semantics used by Vite users. Vite 6 supports Node 18/20/22+, and Node 18 reached EOL on April 30, 2025; plan for node20 soon.
Apply this diff:
- target: 'es2017', + target: 'node18',
- Reference: Vite 6 Node.js support notes, and Node 18 EOL context. (vite.dev)
11-11: Disambiguate outputs with explicit extensions (.cjs/.mjs).When shipping both CJS and ESM from a single outDir, using .cjs for CJS and .mjs for ESM reduces consumer resolution edge-cases and improves Node interop.
Apply this diff:
format: ['cjs', 'esm'], + outExtension({ format }) { + return format === 'cjs' ? { js: '.cjs' } : { js: '.mjs' } + },
- Background on using outExtension with tsup. (blog.logrocket.com)
10-10: Clean the output folder on build.Helps avoid stale artifacts when switching formats/options during iteration.
Apply this diff:
outDir: 'dist', + clean: true,
15-15: Consider always emitting sourcemaps for a library.End-users and your own triage benefit from stack traces mapping back to TS, even in production builds. If bundle size is critical, keep current behavior.
Possible tweak:
- sourcemap: isDev, + sourcemap: true,
16-16: replaceNodeEnv is valid; ensure expectations match behavior.Tsup intentionally stopped auto-replacing process.env.NODE_ENV in v4, but provides an opt-in flag/option. Your use of replaceNodeEnv: true is consistent with that behavior (string replacement at build time). Keep it if your code (or dependencies) branches on NODE_ENV literals; otherwise you can omit it.
- Release note showing the opt-in nature of NODE_ENV replacement. (newreleases.io)
1-1: Nit: You don't need to import 'process' in Node.process is a global in Node. Since the suggestion above removes usage anyway, the import can go.
4-18: Heads‑up: tsup maintenance status and future-proofing.Tsup’s repo notes reduced maintenance and suggests alternatives like tsdown. No action required now, but if build tooling becomes a source of CI friction, consider evaluating migration on your own cadence.
- Reference: tsup repository notice. (github.com)
packages/core/src/utils.ts (1)
116-118: Avoid dumping generated jsCode in thrown errors; log it under debug instead and preserve stack.Throwing the entire code bloats error messages and may leak details. Log it with debug and throw a concise error that includes the original stack.
- catch (error: any) { - throw new Error(`EXEC SCRIPT FAIL IN ${filename}: ${error.message} \n\n${jsCode}\n\n`) - } + catch (error: any) { + if ((debug as any)?.error?.enabled) { + debug.error(`EXEC SCRIPT FAIL IN ${filename}:\n${error?.stack || error?.message}\n\nGenerated code:\n${jsCode}\n`) + } + throw new Error(`EXEC SCRIPT FAIL IN ${filename}: ${error?.message}`) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/core/package.json(3 hunks)packages/core/src/utils.ts(1 hunks)packages/core/tsup.config.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/package.json
🧰 Additional context used
🧬 Code graph analysis (2)
packages/core/tsup.config.ts (2)
packages/core/src/context.ts (1)
PageContext(29-377)packages/core/src/types.ts (2)
Options(18-95)ResolvedOptions(99-118)
packages/core/src/utils.ts (1)
packages/core/src/page.ts (1)
imp(106-106)
🔇 Additional comments (2)
packages/core/tsup.config.ts (2)
9-9: Double-check d.ts generation doesn’t conflict with runtime declaration output.This package also generates declarations at runtime via PageContext.generateDeclaration(options.dts). Ensure tsup’s dts output (dist/*.d.ts) complements rather than collides with that feature (e.g., different paths/filenames, correct "types" field in package.json).
Relevant code that writes declarations at runtime lives in packages/core/src/context.ts (generateDeclaration). Please confirm the published package.json points "types" to the intended API surface (not the runtime-generated declarations).
8-8: Peer dependencies and externals are in sync
Yourexternal: ['vite']entry exactly matches the sole peerDependency (vite) in packages/core/package.json. No changes are required right now. You can still opt into the automated approach later if you add more peerDependencies, but as-is everything is aligned.
| export async function execScript(imports: string[], code: string, filename: string): Promise<any> { | ||
| let jsCode: string = '' | ||
| try { | ||
| const tmpCode = `${imports.join('\n')}\n export default ${code}` | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
API change: execScript now takes (imports, code, filename). Verify all call sites and update JSDoc.
Ensure every usage has been migrated to the 3-arg form and the docblock above reflects the new parameters (especially the semantics of imports and filename resolution).
Run to verify call sites:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "All execScript call sites (context):"
rg -n -C2 --type=ts --type=tsx --type=js --type=jsx '\bexecScript\s*\('
echo
echo "Potential 2-arg calls (need migration):"
rg -nP --type=ts --type=tsx --type=js --type=jsx '\bexecScript\s*\(\s*[^,]+,\s*[^,)]+\s*\)'
echo
echo "3-arg calls (expected):"
rg -nP --type=ts --type=tsx --type=js --type=jsx '\bexecScript\s*\(\s*[^,]+,\s*[^,]+,\s*[^,)]+'Length of output: 204
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "All execScript call sites (context):"
rg -n -C2 --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.jsx' 'execScript\s*\(' .
echo
echo "Potential 2-arg calls (need migration):"
rg -nP --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.jsx' 'execScript\s*\(\s*[^,]+,\s*[^,]+\s*\)' .
echo
echo "3-arg calls (expected):"
rg -nP --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.jsx' 'execScript\s*\(\s*[^,]+,\s*[^,]+,\s*[^,)]+\)' .Length of output: 1028
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Docblock context for execScript (lines with 5 lines of surrounding context):"
rg -nC5 'export async function execScript' packages/core/src/utils.tsLength of output: 615
Documentation update required: JSDoc for execScript missing new parameters
- Call sites have been fully migrated to the 3-argument signature:
- packages/core/src/page.ts:112 uses
execScript(imports, code, sfc.filename) - No remaining 2-arg calls were found in the repo.
- packages/core/src/page.ts:112 uses
- The docblock above
execScriptin packages/core/src/utils.ts (lines 64–68) still only documentscode. It must be updated to include both the newimportsandfilenameparameters.
Suggested JSDoc diff in packages/core/src/utils.ts:
/**
- * 执行 TypeScript 代码字符串并返回其返回值
- * @param code - TypeScript 代码字符串
- * @returns 返回值
+ * 执行 TypeScript 代码字符串并返回其返回值
+ * @param imports - 模块导入语句数组
+ * @param code - TypeScript 代码字符串
+ * @param filename - 源文件名，用于解析模块
+ * @returns 返回值
*/
export async function execScript(imports: string[], code: string, filename: string): Promise<any> {📝 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.
| export async function execScript(imports: string[], code: string, filename: string): Promise<any> { | |
| let jsCode: string = '' | |
| try { | |
| const tmpCode = `${imports.join('\n')}\n export default ${code}` | |
| /** | |
| * 执行 TypeScript 代码字符串并返回其返回值 | |
| * @param imports - 模块导入语句数组 | |
| * @param code - TypeScript 代码字符串 | |
| * @param filename - 源文件名，用于解析模块 | |
| * @returns 返回值 | |
| */ | |
| export async function execScript(imports: string[], code: string, filename: string): Promise<any> { | |
| let jsCode: string = '' | |
| try { | |
| const tmpCode = `${imports.join('\n')}\n export default ${code}` | |
| // … rest of implementation … |
🤖 Prompt for AI Agents
In packages/core/src/utils.ts around lines 69 to 73, the JSDoc above execScript
still documents only the single `code` parameter while the function signature
now takes `imports: string[]`, `code: string`, and `filename: string`; update
the docblock to list and describe all three parameters (imports array of
module/import lines, code string containing the script, and filename string used
for error/context), and update the @returns description if needed to match the
Promise<any> return; ensure parameter names/types match the function signature
exactly.
1 participant
Description 描述
Linked Issues 关联的 Issues
Additional context 额外上下文
Summary by CodeRabbit
New Features
Improvements
Chores