-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
fix(server): include external workspace package roots in fs allow #21703
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| { | ||
| "name": "my-lib" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| { | ||
| "name": "my-app" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "private": true, | ||
| "workspaces": [ | ||
| "my-app", | ||
| "../packages/*" | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| { | ||
| "name": "my-lib" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| { | ||
| "name": "my-app" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| packages: | ||
| - my-app | ||
| - ../packages/* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,13 @@ | ||
| import fs from 'node:fs' | ||
| import { dirname, join } from 'node:path' | ||
| import { | ||
| basename, | ||
| dirname, | ||
| isAbsolute, | ||
| join, | ||
| relative, | ||
| resolve, | ||
| } from 'node:path' | ||
| import picomatch from 'picomatch' | ||
| import { isFileReadable } from '../utils' | ||
|
|
||
| // https://github.com/vitejs/vite/issues/2820#issuecomment-812495079 | ||
|
|
@@ -22,19 +30,143 @@ const ROOT_FILES = [ | |
|
|
||
| // npm: https://docs.npmjs.com/cli/v7/using-npm/workspaces#installing-workspaces | ||
| // yarn: https://classic.yarnpkg.com/en/docs/workspaces/#toc-how-to-use-it | ||
| function hasWorkspacePackageJSON(root: string): boolean { | ||
| function getWorkspacePackagePatterns(root: string): string[] | undefined { | ||
| const path = join(root, 'package.json') | ||
| if (!isFileReadable(path)) { | ||
| return false | ||
| return undefined | ||
| } | ||
| try { | ||
| const content = JSON.parse(fs.readFileSync(path, 'utf-8')) || {} | ||
| return !!content.workspaces | ||
| if (Array.isArray(content.workspaces)) { | ||
| return content.workspaces.filter( | ||
| (workspace: unknown): workspace is string => | ||
| typeof workspace === 'string', | ||
| ) | ||
| } | ||
|
|
||
| if (Array.isArray(content.workspaces?.packages)) { | ||
| return content.workspaces.packages.filter( | ||
| (workspace: unknown) => typeof workspace === 'string', | ||
| ) | ||
| } | ||
|
|
||
| return undefined | ||
| } catch { | ||
| return false | ||
| return undefined | ||
| } | ||
| } | ||
|
|
||
| function getPnpmWorkspacePatterns(root: string): string[] | undefined { | ||
| const path = join(root, 'pnpm-workspace.yaml') | ||
| if (!isFileReadable(path)) { | ||
| return undefined | ||
| } | ||
|
|
||
| try { | ||
| const content = fs.readFileSync(path, 'utf-8') | ||
| const lines = content.split(/\r?\n/) | ||
|
|
||
| const packages: string[] = [] | ||
| let inPackages = false | ||
| let packagesIndent = -1 | ||
|
|
||
| for (const rawLine of lines) { | ||
| const withoutComment = rawLine.replace(/\s+#.*$/, '') | ||
| const line = withoutComment.trimEnd() | ||
| if (!line.trim()) continue | ||
|
|
||
| const indent = withoutComment.length - withoutComment.trimStart().length | ||
| const trimmed = withoutComment.trimStart() | ||
|
|
||
| if (!inPackages) { | ||
| if (trimmed === 'packages:' || trimmed === 'packages: []') { | ||
| inPackages = true | ||
| packagesIndent = indent | ||
| if (trimmed === 'packages: []') { | ||
| return [] | ||
| } | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| if (indent <= packagesIndent) { | ||
| break | ||
| } | ||
|
|
||
| if (!trimmed.startsWith('- ')) { | ||
| continue | ||
| } | ||
|
|
||
| const pattern = trimmed.slice(2).trim() | ||
| if (!pattern) continue | ||
|
|
||
| const unquoted = | ||
| (pattern.startsWith('"') && pattern.endsWith('"')) || | ||
| (pattern.startsWith("'") && pattern.endsWith("'")) | ||
| ? pattern.slice(1, -1) | ||
| : pattern | ||
|
|
||
| packages.push(unquoted) | ||
| } | ||
|
|
||
| return packages.length ? packages : undefined | ||
| } catch { | ||
| return undefined | ||
| } | ||
| } | ||
|
|
||
| function getWorkspacePackageBases(root: string, patterns: string[]): string[] { | ||
| const bases = patterns | ||
| .filter((pattern) => !pattern.startsWith('!')) | ||
| .map((pattern) => { | ||
| let { base } = picomatch.scan(pattern) | ||
| if (!base || base === '.') { | ||
| return root | ||
| } | ||
|
|
||
| if (basename(base).includes('.')) { | ||
| base = dirname(base) | ||
| } | ||
|
|
||
| return resolve(root, base) | ||
| }) | ||
|
|
||
| return [root, ...bases] | ||
| } | ||
|
|
||
| function getCommonAncestor(paths: string[]): string { | ||
| if (!paths.length) { | ||
| return '' | ||
| } | ||
|
|
||
| let ancestor = resolve(paths[0]) | ||
| for (const path of paths.slice(1).map((path) => resolve(path))) { | ||
| while (ancestor !== dirname(ancestor)) { | ||
| const relation = relative(ancestor, path) | ||
| if ( | ||
| relation === '' || | ||
| (!relation.startsWith('..') && !isAbsolute(relation)) | ||
| ) { | ||
| break | ||
| } | ||
| ancestor = dirname(ancestor) | ||
| } | ||
| } | ||
|
Comment on lines
+137
to
+154
|
||
|
|
||
| return ancestor | ||
| } | ||
|
|
||
| function getWorkspaceRoot(root: string): string | undefined { | ||
| const patterns = | ||
| getPnpmWorkspacePatterns(root) ?? getWorkspacePackagePatterns(root) | ||
| if (!patterns?.length) { | ||
| return undefined | ||
| } | ||
|
|
||
| const packageBases = getWorkspacePackageBases(root, patterns) | ||
| return getCommonAncestor(packageBases) | ||
| } | ||
|
|
||
| function hasRootFile(root: string): boolean { | ||
| return ROOT_FILES.some((file) => fs.existsSync(join(root, file))) | ||
| } | ||
|
|
@@ -67,8 +199,9 @@ export function searchForWorkspaceRoot( | |
| current: string, | ||
| root: string = searchForPackageRoot(current), | ||
| ): string { | ||
| const workspaceRoot = getWorkspaceRoot(current) | ||
| if (workspaceRoot) return workspaceRoot | ||
| if (hasRootFile(current)) return current | ||
| if (hasWorkspacePackageJSON(current)) return current | ||
|
|
||
| const dir = dirname(current) | ||
| // reach the fs root | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
The heuristic
basename(base).includes('.')assumes that paths with dots in the basename are files rather than directories. While this works for typical cases, directories can have dots in their names (e.g.,packages/v1.0/). This could cause the function to incorrectly use the parent directory. Consider checking if the path actually exists on the filesystem and is a directory, or documenting this limitation. However, the impact is limited: it would just make the allow list slightly more permissive than necessary, which is safe.