-
Notifications
You must be signed in to change notification settings - Fork 29k
feat: add validation for root-level files when src directory exists #82514
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: canary
Are you sure you want to change the base?
feat: add validation for root-level files when src directory exists #82514
Conversation
- Add SrcDirectoryError class for consistent error messaging - Add validateMiddlewareInSrcDir and validateInstrumentationInSrcDir functions - Integrate validation in both dev bundler and build processes - Throw error when middleware/instrumentation files are outside src/ when src/ directory exists - Add test cases to verify validation behavior Co-Authored-By: Jiwon Choi <devjiwonchoi@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
Allow CI Workflow Run
Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer |
packages/next/src/build/utils.ts
Outdated
} | ||
|
||
export function validateMiddlewareInSrcDir(fileName: string, isSrcDir: boolean) { | ||
if (isSrcDir && isMiddlewareFile(fileName) && !fileName.startsWith('/src/')) { |
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 new validation functions use hardcoded /src/
path prefixes which will fail on Windows systems where paths use backslashes, causing false positive errors even when files are correctly placed in the src directory.
View Details
Analysis
The validateMiddlewareInSrcDir
and validateInstrumentationInSrcDir
functions check if file paths start with /src/
using fileName.startsWith('/src/')
. However, in the build process, the rootPaths
are created using path.join(rootDir, file).replace(dir, '')
which preserves platform-specific path separators.
On Windows systems, this creates paths like \src\middleware.js
(with backslashes), but the validation checks for /src/
(with forward slashes). This means:
- On Windows: A middleware file correctly placed in
src/middleware.js
will have a path like\src\middleware.js
- The validation:
!fileName.startsWith('/src/')
will returntrue
(since\src\middleware.js
doesn't start with/src/
) - Result: The validation throws
SrcDirectoryError
even though the file is in the correct location
This is inconsistent with the dev server code path, which uses normalizePathSep()
to convert backslashes to forward slashes before validation, ensuring cross-platform compatibility.
The codebase has established patterns for handling cross-platform paths (see normalizePathSep()
, ensureLeadingSlash()
functions), but the new validation functions don't follow these patterns.
Recommendation
Normalize the path separators before validation to ensure cross-platform compatibility. Update both validation functions to use forward slashes consistently:
export function validateMiddlewareInSrcDir(fileName: string, isSrcDir: boolean) {
const normalizedPath = fileName.replace(/\\/g, '/')
if (isSrcDir && isMiddlewareFile(fileName) && !normalizedPath.startsWith('/src/')) {
throw new SrcDirectoryError(fileName, 'middleware')
}
}
export function validateInstrumentationInSrcDir(fileName: string, isSrcDir: boolean) {
const normalizedPath = fileName.replace(/\\/g, '/')
if (isSrcDir && isInstrumentationHookFile(fileName) && !normalizedPath.startsWith('/src/')) {
throw new SrcDirectoryError(fileName, 'instrumentation')
}
}
Alternatively, import and use the existing normalizePathSep
function from shared/lib/page-path/normalize-path-sep.ts
for consistency with the rest of the codebase.
Co-Authored-By: Jiwon Choi <devjiwonchoi@gmail.com>
packages/next/src/build/index.ts
Outdated
if (rootPath.includes(MIDDLEWARE_FILENAME)) { | ||
validateMiddlewareInSrcDir(rootPath, isSrcDir) | ||
} | ||
if (rootPath.includes(INSTRUMENTATION_HOOK_FILENAME)) { | ||
validateInstrumentationInSrcDir(rootPath, isSrcDir) | ||
} |
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 src directory validation for middleware and instrumentation files never triggers during builds because the validation functions expect paths without file extensions, but receive paths with extensions.
View Details
Analysis
The build process creates rootPaths
that contain file extensions (like /middleware.js
, /src/middleware.js
) through this processing:
const rootPaths = Array.from(await getFilesInDir(rootDir))
.filter((file) => includes.some((include) => include.test(file)))
.map((file) => path.join(rootDir, file).replace(dir, ''))
However, the validation functions validateMiddlewareInSrcDir()
and validateInstrumentationInSrcDir()
rely on isMiddlewareFile()
and isInstrumentationHookFile()
which only match exact paths without extensions:
export function isMiddlewareFile(file: string) {
return (
file === `/${MIDDLEWARE_FILENAME}` || file === `/src/${MIDDLEWARE_FILENAME}`
)
}
This means isMiddlewareFile('/middleware.js')
returns false
, causing the validation condition to never trigger. The validation works correctly in development mode because absolutePathToPage()
strips file extensions, but fails during builds.
This bug means the new validation feature is completely non-functional during builds, potentially allowing users to incorrectly place middleware/instrumentation files outside the src directory when they should be required to be inside it.
Recommendation
Strip the file extension from the path before calling the validation functions. Modify the validation calls in lines 1167-1172:
if (rootPath.includes(MIDDLEWARE_FILENAME)) {
const pathWithoutExtension = rootPath.replace(/\.[^/.]+$/, '')
validateMiddlewareInSrcDir(pathWithoutExtension, isSrcDir)
}
if (rootPath.includes(INSTRUMENTATION_HOOK_FILENAME)) {
const pathWithoutExtension = rootPath.replace(/\.[^/.]+$/, '')
validateInstrumentationInSrcDir(pathWithoutExtension, isSrcDir)
}
Alternatively, update the validation functions to handle paths with extensions by modifying isMiddlewareFile
and isInstrumentationHookFile
to strip extensions before comparison.
…ion issues - Use normalizePathSep for cross-platform path handling - Strip file extensions before validation in build context - Ensure validation works consistently in dev and build modes Co-Authored-By: Jiwon Choi <devjiwonchoi@gmail.com>
@@ -1157,6 +1161,19 @@ export default async function build( | |||
|
|||
NextBuildContext.hasInstrumentationHook = hasInstrumentationHook | |||
|
|||
if (isSrcDir) { |
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 build process fails to detect middleware and instrumentation files placed at the project root when using src directory structure, preventing helpful validation errors from being shown to users.
View Details
Analysis
When using a src directory structure (src/pages
or src/app
), the build process should detect if middleware or instrumentation files are incorrectly placed at the project root instead of inside the src/
directory, and throw a clear error to guide users.
However, there's a bug in the file detection logic on line 1144. The code calculates:
const rootDir = path.join((pagesDir || appDir)!, '..')
When pagesDir
is /project/src/pages
, this results in rootDir = /project/src
. The subsequent getFilesInDir(rootDir)
call only searches within the src directory, completely missing files that are incorrectly placed at the actual project root (/project/
).
This means users who place middleware.js
or instrumentation.js
at the project root while using src directory structure won't receive the helpful build-time validation error. Instead, they'll experience confusing runtime behavior without clear guidance on where to place their files.
The test case "should throw error when middleware is outside src directory" expects this validation to work, confirming this is a bug rather than intended behavior.
Recommendation
Change the rootDir calculation to properly search the project root directory. Replace line 1144 with:
const projectRoot = path.join((pagesDir || appDir)!, '../..')
const srcRoot = path.join((pagesDir || appDir)!, '..')
const rootDir = isSrcDir ? projectRoot : srcRoot
Or alternatively, search both locations when isSrcDir
is true:
const searchDirs = isSrcDir ?
[path.join((pagesDir || appDir)!, '../..'), path.join((pagesDir || appDir)!, '..')] :
[path.join((pagesDir || appDir)!, '..')]
This ensures that when using src directory structure, the validation checks both the project root (to catch incorrectly placed files) and the src directory (to allow correctly placed files).
What?
This PR adds validation to throw an error when root-level files (
middleware
andinstrumentation
) are found outside thesrc/
directory when asrc/
directory structure is being used.Why?
Currently, Next.js allows middleware and instrumentation files to exist at the project root even when using a
src/
directory structure. This can lead to confusion and inconsistent file organization. The Next.js documentation states that when using the src directory, middleware should be placed inside the src folder, but this wasn't enforced.How?
Added new error class:
SrcDirectoryError
inpackages/next/src/build/utils.ts
that provides clear error messaging and documentation linksAdded validation functions:
validateMiddlewareInSrcDir(fileName: string, isSrcDir: boolean)
validateInstrumentationInSrcDir(fileName: string, isSrcDir: boolean)
Integrated validation in two key locations:
packages/next/src/server/lib/router-utils/setup-dev-bundler.ts
- validates during dev server startuppackages/next/src/build/index.ts
- validates during production buildAdded test cases: Created
test/integration/src-dir-validation/
to verify the validation behaviorThe validation logic checks:
src/
directory structure is being used (isSrcDir
)src/
directorySrcDirectoryError
with helpful guidanceCritical items to verify:
fileName.startsWith('/src/')
correctly identifies files inside src directory for Next.js internal file pathstest/integration/src-dir-validation/
to ensure they work correctlytest/integration/middleware-src
)next dev
andnext build
scenariosBreaking Change Consideration: This introduces new fatal errors for previously "working" (but incorrectly organized) projects. Consider if this should be a warning first before becoming an error in a future version.
Link to Devin run: https://app.devin.ai/sessions/94e9f081963a4ec3bc8bc9c8c1ca8975
Requested by: @devjiwonchoi