-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
feat(compiler-sfc): support Node subpath imports for type resolution #13813
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?
feat(compiler-sfc): support Node subpath imports for type resolution #13813
Conversation
WalkthroughAdds Node subpath import resolution fallback in type resolver using resolve.exports, integrates it into importSourceToScope after TS resolution, introduces helpers to locate/read nearest package.json, and expands tests to cover node-style subpath imports. Also adds a devDependency on resolve.exports in package.json. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SFC as SFC Compiler
participant RTS as resolveType.importSourceToScope
participant TS as resolveWithTS
participant NSI as resolveWithNodeSubpathImports
participant FS as FS
participant RE as resolve.exports
SFC->>RTS: resolve import source for types
RTS->>TS: attempt TS-based resolution
alt TS resolves
TS-->>RTS: resolved path
RTS-->>SFC: resolved and scoped types
else TS fails
note over RTS,NSI: Fallback (CommonJS only)
RTS->>NSI: resolve via Node subpath imports
NSI->>FS: findPackageJsonFile (walk up)
FS-->>NSI: package.json path (or none)
alt package.json found
NSI->>FS: read package.json
NSI->>RE: resolveExports(imports map, specifier)
RE-->>NSI: mapped subpath
NSI->>FS: realpath (optional)
NSI-->>RTS: resolved path (if successful)
RTS-->>SFC: resolved and scoped types
else not found or mapping fails
NSI-->>RTS: no resolution
RTS-->>SFC: unresolved (existing handling)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes(none) Suggested labels
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 (
|
Size ReportBundles
Usages
|
@vue/compiler-core
@vue/compiler-dom
@vue/compiler-sfc
@vue/compiler-ssr
@vue/reactivity
@vue/runtime-core
@vue/runtime-dom
@vue/server-renderer
@vue/shared
vue
@vue/compat
commit: |
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: 5
🧹 Nitpick comments (2)
packages/compiler-sfc/src/script/resolveType.ts (2)
1159-1173
: Consider caching package.json for performance.The
findPackageJsonFile
function searches for package.json by traversing the directory tree on every call. Since package.json files rarely change during runtime, consider implementing a simple cache to avoid repeated filesystem operations.Here's a suggested implementation with caching:
+const packageJsonCache = new Map<string, string | undefined>() + function findPackageJsonFile(fs: FS): string | undefined { + const cacheKey = process.cwd() + if (packageJsonCache.has(cacheKey)) { + return packageJsonCache.get(cacheKey) + } + let currDir = process.cwd() while (true) { const filePath = joinPaths(currDir, 'package.json') if (fs.fileExists(filePath)) { + packageJsonCache.set(cacheKey, filePath) return filePath } const parentDir = dirname(currDir) if (parentDir === currDir) { + packageJsonCache.set(cacheKey, undefined) return } currDir = parentDir } }
1152-1152
: Consider using joinPaths for consistency.The code uses
resolve
from the path module here, but usesjoinPaths
elsewhere in the codebase for path operations. Consider usingjoinPaths
for consistency with the rest of the codebase.- const resolved = resolve(dirname(pkgPath), resolvedImports[0]) + const resolved = joinPaths(dirname(pkgPath), resolvedImports[0])
📜 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.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/compiler-sfc/__tests__/compileScript/resolveType.spec.ts
(1 hunks)packages/compiler-sfc/package.json
(1 hunks)packages/compiler-sfc/src/script/resolveType.ts
(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/compiler-sfc/src/script/resolveType.ts (1)
packages/compiler-sfc/src/script/utils.ts (1)
joinPaths
(111-112)
🪛 GitHub Actions: ci
packages/compiler-sfc/__tests__/compileScript/resolveType.spec.ts
[error] 1-1: Test 'external type imports > node subpath imports' failed while running 'pnpm run test-unit': [@vue/compiler-sfc] Failed to resolve import source '#t1'.
⏰ 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). (3)
- GitHub Check: Redirect rules
- GitHub Check: Header rules
- GitHub Check: Pages changed
🔇 Additional comments (2)
packages/compiler-sfc/src/script/resolveType.ts (1)
42-45
: Incorrect import usage of resolve.exports.The destructured import
{ imports as resolveImports }
is incorrect. According to theresolve.exports
documentation, the correct import should be{ imports }
and it should be used asimports()
notresolveImports()
.-import { imports as resolveImports } from 'resolve.exports' +import { imports } from 'resolve.exports'Likely an incorrect or invalid review comment.
packages/compiler-sfc/__tests__/compileScript/resolveType.spec.ts (1)
1201-1235
: Test correctly validates the new Node subpath imports feature.The test effectively covers the main functionality:
- Maps
#t1
to./t1.ts
(relative path)- Maps
#t2
to/t2.ts
(absolute path)- Maps wildcard pattern
#o/*.ts
to./other/*.ts
- Verifies type resolution and dependency tracking
@@ -1198,6 +1198,42 @@ describe('resolveType', () => { | |||
expect(deps && [...deps]).toStrictEqual(['/user.ts']) | |||
}) | |||
|
|||
test('node subpath imports', () => { |
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.
💡 Verification agent
🧩 Analysis chain
Test failure indicates the feature isn't working as expected.
Based on the pipeline failure message "Failed to resolve import source '#t1'", the Node subpath imports resolution is not working correctly. This could be due to:
- The incorrect import statement for
resolve.exports
(as noted in the other file) - The package being in devDependencies instead of dependencies
- Missing
__CJS__
flag during test execution
Let me verify the test environment setup:
🏁 Script executed:
#!/bin/bash
# Check if __CJS__ is defined in the test environment
rg -n "__CJS__" packages/compiler-sfc/__tests__/ -A 2 -B 2
# Check how the tests are built and run
fd "jest.config" --type f | xargs cat 2>/dev/null
# Check the build configuration for tests
fd "rollup.config" --type f | head -5 | xargs grep -l "compiler-sfc" 2>/dev/null | xargs cat 2>/dev/null | grep -A 10 -B 10 "__CJS__"
Length of output: 993
🏁 Script executed:
#!/bin/bash
# Search for usages of resolve.exports in the compiler-sfc package
rg -n "resolve\\.exports" -C3 packages/compiler-sfc
# Inspect dependencies and devDependencies in the compiler-sfc package.json
grep -n "\"dependencies\"" -A20 packages/compiler-sfc/package.json
grep -n "\"devDependencies\"" -A20 packages/compiler-sfc/package.json
Length of output: 2326
🏁 Script executed:
#!/bin/bash
# Show code around __CJS__ usage in resolveType.ts
rg -n "__CJS__" -C5 packages/compiler-sfc/src/script/resolveType.ts
# Show how resolveImports is invoked
rg -n "resolveImports" -C5 packages/compiler-sfc/src/script/resolveType.ts
# Inspect the failing test for '#t1' import source
rg -n "'#t1'" -C5 packages/compiler-sfc/__tests__/compileScript/resolveType.spec.ts
Length of output: 2807
Ensure Node subpath imports are resolved at runtime and in tests
The failure “Failed to resolve import source ‘#t1’” is expected:
- At runtime,
resolve.exports
is only listed under devDependencies, so it isn’t available when this code path executes. - In the unbundled test harness, the
__CJS__
guard is falsey by default (no global definition), soresolveWithNodeSubpathImports
immediately returns without attempting subpath resolution.
To fix this:
- Move
resolve.exports
intodependencies
so it’s always installed at runtime. - Define
__CJS__ = true
in the test environment (e.g. via a Jest setup file orglobals
in your test config) so the Node‐only resolution path is exercised. - No change is needed to the import statement (
import { imports as resolveImports } from 'resolve.exports'
is correct).
Locations to update:
- packages/compiler-sfc/package.json
"dependencies": { "@babel/parser": "catalog:", // …
- },
- "devDependencies": {
- "resolve.exports": "^2.0.3",
- }
+}, - "dependencies": {
- "resolve.exports": "^2.0.3",
- // …
+}, - "devDependencies": {
- // other dev deps…
// …
}
- **Test setup** (e.g. add a `setupTests.ts` or update Jest/Vitest config)
```ts
// packages/compiler-sfc/__tests__/setupTests.ts
;(global as any).__CJS__ = true
and ensure it’s loaded before tests (via your test runner’s setupFiles
/setup
option).
These changes will install the resolver at runtime and unblock the resolveWithNodeSubpathImports
logic in tests.
🤖 Prompt for AI Agents
In packages/compiler-sfc/__tests__/compileScript/resolveType.spec.ts around line
1201, the test fails because resolve.exports is only in devDependencies and the
test runtime doesn't set the __CJS__ global, so Node subpath resolution path
isn't executed; fix by moving "resolve.exports" from devDependencies to
dependencies in packages/compiler-sfc/package.json so it is installed at
runtime, and add a test setup file (or update your Jest/Vitest config's
setupFiles) that sets global.__CJS__ = true before tests run so
resolveWithNodeSubpathImports is exercised (no change to the import statement is
required).
@@ -62,6 +62,7 @@ | |||
"postcss-modules": "^6.0.1", | |||
"postcss-selector-parser": "^7.1.0", | |||
"pug": "^3.0.3", | |||
"resolve.exports": "^2.0.3", |
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.
Should be a regular dependency, not a devDependency.
The resolve.exports
package is used in production code (src/script/resolveType.ts
) to resolve Node.js subpath imports. Since it's required at runtime when the package is used by consumers, it should be listed under dependencies
rather than devDependencies
.
Apply this fix to move it to the correct section:
"dependencies": {
"@babel/parser": "catalog:",
"@vue/compiler-core": "workspace:*",
"@vue/compiler-dom": "workspace:*",
"@vue/compiler-ssr": "workspace:*",
"@vue/shared": "workspace:*",
"estree-walker": "catalog:",
"magic-string": "catalog:",
"postcss": "^8.5.6",
+ "resolve.exports": "^2.0.3",
"source-map-js": "catalog:"
},
"devDependencies": {
"@babel/types": "catalog:",
"@vue/consolidate": "^1.0.0",
"hash-sum": "^2.0.0",
"lru-cache": "10.1.0",
"merge-source-map": "^1.1.0",
"minimatch": "~10.0.3",
"postcss-modules": "^6.0.1",
"postcss-selector-parser": "^7.1.0",
"pug": "^3.0.3",
- "resolve.exports": "^2.0.3",
"sass": "^1.90.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.
"resolve.exports": "^2.0.3", | |
"dependencies": { | |
"@babel/parser": "catalog:", | |
"@vue/compiler-core": "workspace:*", | |
"@vue/compiler-dom": "workspace:*", | |
"@vue/compiler-ssr": "workspace:*", | |
"@vue/shared": "workspace:*", | |
"estree-walker": "catalog:", | |
"magic-string": "catalog:", | |
"postcss": "^8.5.6", | |
"resolve.exports": "^2.0.3", | |
"source-map-js": "catalog:" | |
}, | |
"devDependencies": { | |
"@babel/types": "catalog:", | |
"@vue/consolidate": "^1.0.0", | |
"hash-sum": "^2.0.0", | |
"lru-cache": "10.1.0", | |
"merge-source-map": "^1.1.0", | |
"minimatch": "~10.0.3", | |
"postcss-modules": "^6.0.1", | |
"postcss-selector-parser": "^7.1.0", | |
"pug": "^3.0.3", | |
"sass": "^1.90.0" | |
} |
🤖 Prompt for AI Agents
In packages/compiler-sfc/package.json around line 65, the dependency
"resolve.exports": "^2.0.3" is currently listed as a devDependency but is
required at runtime (used in src/script/resolveType.ts); move the
"resolve.exports": "^2.0.3" entry from devDependencies to dependencies in
package.json and remove it from devDependencies, then run your package manager
(npm/yarn/pnpm) to update the lockfile so consumers get the runtime dependency.
resolved = | ||
resolveWithTS(scope.filename, source, ts, fs) || | ||
resolveWithNodeSubpathImports(source, fs) |
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.
Fix reference to renamed import.
Update the call site to use the correct import name after fixing the import statement.
resolved =
resolveWithTS(scope.filename, source, ts, fs) ||
- resolveWithNodeSubpathImports(source, fs)
+ resolveWithNodeSubpathImports(source, fs)
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/compiler-sfc/src/script/resolveType.ts around lines 962 to 964, the
call site still uses the old name resolveWithNodeSubpathImports; update this
invocation to match the renamed import (use the exact new exported identifier
from the top-of-file import), e.g. replace resolveWithNodeSubpathImports with
the corrected import name and ensure the function is called with the same
parameters (source, fs) as before so the fallback resolution chain remains:
resolveWithTS(...) || <correctRename>(source, fs).
function resolveWithNodeSubpathImports( | ||
source: string, | ||
fs: FS, | ||
): string | undefined { | ||
if (!__CJS__) return | ||
|
||
try { | ||
const pkgPath = findPackageJsonFile(fs) | ||
if (!pkgPath) { | ||
return | ||
} | ||
|
||
const pkgStr = fs.readFile(pkgPath) | ||
if (!pkgStr) { | ||
return | ||
} | ||
|
||
const pkg = JSON.parse(pkgStr) | ||
const resolvedImports = resolveImports(pkg, source) | ||
if (!resolvedImports || !resolvedImports.length) { | ||
return | ||
} | ||
|
||
const resolved = resolve(dirname(pkgPath), resolvedImports[0]) | ||
|
||
return fs.realpath ? fs.realpath(resolved) : resolved | ||
} catch (e) {} | ||
} |
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.
🛠️ Refactor suggestion
Add validation for package.json imports field.
The function assumes that pkg.imports
exists and that resolvedImports
returns an array. Add validation to handle edge cases where the imports field might be missing or malformed.
function resolveWithNodeSubpathImports(
source: string,
fs: FS,
): string | undefined {
if (!__CJS__) return
try {
const pkgPath = findPackageJsonFile(fs)
if (!pkgPath) {
return
}
const pkgStr = fs.readFile(pkgPath)
if (!pkgStr) {
return
}
const pkg = JSON.parse(pkgStr)
+ if (!pkg.imports || typeof pkg.imports !== 'object') {
+ return
+ }
+
- const resolvedImports = imports(pkg, source)
+ const resolvedImports = imports(pkg, source)
if (!resolvedImports || !resolvedImports.length) {
return
}
const resolved = resolve(dirname(pkgPath), resolvedImports[0])
return fs.realpath ? fs.realpath(resolved) : resolved
} catch (e) {}
}
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/compiler-sfc/src/script/resolveType.ts around lines 1129 to 1156,
the code assumes pkg.imports exists and that resolveImports returns an array;
add defensive validation: after parsing package.json validate that pkg && typeof
pkg === 'object' and that pkg.imports exists and is an object (or of the
expected shape) before calling resolveImports; call resolveImports only if
imports look valid, then ensure its return is an array and has at least one
element before using resolvedImports[0]; if validations fail, return undefined
early; also avoid swallowing errors silently — keep the try/catch but return
undefined on error (or rethrow/log if preferred).
} | ||
|
||
const pkg = JSON.parse(pkgStr) | ||
const resolvedImports = resolveImports(pkg, source) |
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.
Update to use correct import name from resolve.exports.
After fixing the import statement, update this line to use the correct function name.
- const resolvedImports = resolveImports(pkg, source)
+ const resolvedImports = imports(pkg, source)
📝 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 resolvedImports = resolveImports(pkg, source) | |
const resolvedImports = imports(pkg, source) |
🤖 Prompt for AI Agents
In packages/compiler-sfc/src/script/resolveType.ts around line 1147, the call
currently uses resolveImports(pkg, source) but the correct function name from
resolve.exports is resolveExports; change this invocation to resolveExports(pkg,
source) and ensure the file's import statement at the top imports resolveExports
(not resolveImports) from resolve.exports so the symbol matches.
close #13586
Support reading user-defined Node.js subpath imports for type resolution.
Example:
Comp1.vue
Comp2.vue
package.json
Summary by CodeRabbit