Resolve build issues with symlinked files #57412
Conversation
LavaToaster
requested review from
timneutkens,
ijjk,
shuding,
huozhi,
feedthejim,
ztanner and
wyattjoh
as code owners
LavaToaster
force-pushed
the
patch-2
branch
5 times, most recently
from
171b3fb
to
0c94702
Compare
|
@timneutkens @styfle Any chance of this getting a review? ❤️ |
|
@timneutkens When I created the PR I did check to see if there were tests for this, but I couldn't find any. More than happy to introduce some if you can show me the right places, or if it can be tested indirectly easily enough. |
|
Given that you pointed out there's a case where you know it will fail you can add a test for that particular case using |
LavaToaster
changed the title
flatReaddir ignoring include condition on symbolic files
| if (file.isFile()) { | ||
| fileNames.add(file.name) | ||
| } | ||
| } |
There was a problem hiding this comment.
When writing tests I noticed that this area suffered form the same issue with symbolic links. It wouldn't recognise that layout.tsx existed and would complain that page.tsx doesn't have a root layout file..
Rather than fix it is, I decided re-use the now getFilesInDir. This played a role in dropping the includes predicate check from getFilesInDir.
| ] | ||
|
|
||
| const rootPaths = (await getFilesInDir(rootDir)) | ||
| .filter((file) => includes.some((include) => include.test(file))) |
There was a problem hiding this comment.
I removed the predicate includes check from the updated getFilesInDir function. I felt it would be just as easy for this area to filter out what it didn't needed.
| .sort(sortByPageExts(config.pageExtensions)) | ||
| .map((absoluteFile) => absoluteFile.replace(dir, '')) |
There was a problem hiding this comment.
Small amount of irony in that previously flatReaddir returned an absolute path which is then iterated through again to remove. Probably for historical reasons it used to have to do that, but the updated function no longer returns an absolute path so no need.
There was a problem hiding this comment.
😅 This was the cause of test failures since I didn't realise that that the end result would be ["/middleware.ts"] and instead returned ["middleware.ts"]. I've added it back in.
|
@timneutkens I have added a unit and e2e test. Identified another symlink issue when I was setting up the e2e test so I've also resolved that, not too sure on how I hadn't encountered it before this. (Unless that code was post v14 since we haven't upgraded yet) |
| import fs from 'fs/promises' | ||
| import type { Dirent, StatsBase } from 'fs' | ||
|
|
||
| export async function getFilesInDir(path: string): Promise<string[]> { |
There was a problem hiding this comment.
I renamed this to something more fitting, since I couldn't really understand what flatReaddir was supposed to reference unless historically it was recursive and did indeed flatten something.
This resolves an issue introduced in vercel#52361 where `shouldOmit` is overwritten based on if it is a directory or not without considering include conditions. In an environment where the the application being built is orchestrated and managed by bazel, all files are symbolic links and thus without this the file includes predicate checks aren't considered - and the build fails under certain conditions.
…loader, add tests
|
I can't tell if test failures are my issue to resolve or not. That is to say that I don't know if I caused them, especially as most of them look like timeout errors. |
|
Test failures were correct. I've issued a fix, looks like the Azure Pipelines are happy now :) |
|
@timneutkens any chance you could trigger the gh workflow again, and give this a look over now? 😄 |
Tests Passed |
Stats from current PRDefault BuildGeneral
Client Bundles (main, webpack)
Legacy Client Bundles (polyfills)
Client Pages
Client Build Manifests
Rendered Page Sizes
Edge SSR bundle Size
Middleware size
Next Runtimes
|
|
GitHub CI should pass now. Though I will note that |
|
Someone able to re-run that last(?) failing workflow. Failed due to a git checkout issue. |
|
😅 need another run, the error now is for an address already in use. |
|
Anything else I need to sort out? 👀 |
|
Thanks @LavaToaster! |
This resolves an issue introduced in #52361 where
shouldOmitis overwritten based on if it is a directory or not without considering include conditions.In an environment where the the application being built is orchestrated and managed by bazel, all files are symbolic links and thus without this the file includes predicate checks aren't considered - and the build fails under certain conditions.