Fix Windows path handling and refuse to clean what cannot be identified - #69
Merged
Conversation
…orms The plugin matches esbuild's metafile keys, which use forward slashes and are relative to absWorkingDir, against path.relative(process.cwd(), file). Where those two disagree every built file reads as unused, so with destructive: true the plugin deletes the build it just finished. The tests are expected to fail until that is fixed. The absWorkingDir case already fails on macOS and Linux; the separator case needs the Windows runner the new matrix adds. The diagnostic steps and the metafile probe exist to make that run readable and can come out once the behaviour is settled.
The plugin matches names exactly, so a stale output whose name differs from a built one only by case means different things per filesystem. On Linux the two are separate files and the stale one is genuinely unused. On macOS and Windows they are one file, and the case-preserving directory entry keeps the stale spelling, so the plugin reads the file esbuild just wrote as unused and deletes it. The built file surviving is the expectation on every platform. It already fails on macOS, which puts this defect on more than just Windows.
The plugin compared esbuild's metafile keys against path.relative output. Those disagree in three ways: esbuild always writes forward slashes, its keys are relative to absWorkingDir rather than to the process working directory, and a case-folding filesystem reports a stored name that need not match the one esbuild wrote. Each disagreement marked a freshly built file as unused, so with destructive: true the plugin deleted the build it had just finished. Asking the filesystem whether two paths are the same file settles all three at once, with no platform switch and no guess about case sensitivity. The output directory now resolves against esbuild's working directory as well, so a relative outdir under a different absWorkingDir no longer walks an unrelated project's output.
… Windows The adapter read two native paths with posix semantics and used the result as an ES import specifier. On Windows that is wrong twice over: posix.relative finds no common root in backslash paths so it emits the whole absolute target, and the backslashes it keeps are then consumed as escape sequences when esbuild parses the generated source. That is the "D:aecosystemecosystem" resolution failure in the Windows build. The computation moves into a named function taking the path semantics as an argument, defaulting to posix, so nothing behaves differently yet. Two tests fail: the separator is never converted, which fails everywhere, and posix semantics are wrong for native paths, which only a Windows runner can show.
The import specifier is now built with the platform's own relative, then converted to forward slashes for the generated source. Reading native Windows paths as posix ones produced a specifier carrying the whole absolute target and its backslashes, which esbuild then failed to resolve. The single-argument join at the call site went with it. It normalised a path that relative normalises anyway.
The glob results these tests check go to esbuild's watchFiles and vite's addWatchFile, which take paths in the platform's own form. Nothing compares them against the forward-slashed paths findGraphQLFiles produces for generated source, so the separator carries no meaning here and the tests should not have been pinning it. Which files are registered is the behaviour, and that is what they assert now. The root build and test scripts also run to completion rather than stopping at the first failing task. One package failing was cancelling the rest, which hid every other package's result on a platform and cost two runs before the Windows column could be read at all.
The readme showed a vite config as a working example. The vite entry point, like rollup, webpack, rspack, farm, rolldown, nuxt and astro, registers no hook that could clean, so that example never removed a file. The options block was also missing features and logger, both shipped in 1.2.0.
shellicar
changed the title
shellicar
marked this pull request as ready for review
August 27, 2026 04:02
… trusted Three cases where the plugin could not know what it was looking at and deleted anyway: none of the build's outputs present under the directory it was given, the directory unreadable rather than absent, and a directory outside the build altogether. Each now removes nothing and says why. A new strict option turns that refusal into a build failure, off by default so an upgrade cannot start breaking builds. The containment guard could not see a directory on another drive or a UNC share, because path.relative returns an absolute path when no relative route exists and the check only looked for a leading "..". The absolute-path guard beside it could never fire at all, since resolving an absolute path against any base returns it unchanged. One condition replaces both. Refusals name the output directory as it was configured again, which also restores what the source-directory check reads. A symlink pointing at a build output is removed, by identifying files with lstat rather than stat. A hard link to one is kept and now says so in the readme: both names are equally the file esbuild wrote, and telling them apart means returning to the path comparison that deleted builds on Windows. The test workspaces move out of the repository into the OS temp directory, where scratch output belongs. They had been written into the package's own test directory, behind a gitignore entry that kept them out of sight.
A failed build writes no metafile, and the plugin read that as the metafile option being off, telling the user to enable something it turns on itself. Every compile error came with a second error saying so. It now returns when the build reports errors, and keeps the diagnostic for the case it actually describes: a build that succeeded with the option removed underneath it. Two faults in the refusal path, both reached only by the branch added in the previous commit and neither covered by a test. An absent cause was still passed to the logger, which spreads its arguments, so the refusal ended in the word undefined. And a refusal under strict was caught by the surrounding handler and logged a second time wrapped in its own message. A build reporting no outputs still refuses rather than emptying the directory, which is the case this guard exists for, but it no longer says "of the 0 the build reported". The change entries are rewritten as changelog lines rather than prose, and gain one that was missing: a hard link to a build output used to be removed and now is not.
Refusing there was new in this branch and wrong. The guard reads zero matches as the wrong directory, and that only carries information when there was something to match: a build that produced nothing matches nothing wherever it is pointed. Everything present came from an earlier build, which is exactly what this plugin removes, and leaving it means a stale output can be deployed as current. The rest is the coverage that would have caught it, and three other branches that had none. The default logger had no test at all, which is how a refusal came to print the word undefined to users. Neither the missing output directory of a first build, nor the metafile and outdir the hook refuses to run without, nor a file that cannot be deleted, nor the working-directory fallback taken by every build that does not set absWorkingDir. removeEmptyDirs guards reading a directory and reports the failure, then reads it again unguarded, so the failure surfaces as a throw anyway. Pinned as it stands rather than changed. Branch coverage of the core is 96%. What is left is the handler that logs and rethrows an unexpected error, which needs an injected fault to reach and alters nothing on the way through.
Every failure was a bare Error carrying a sentence, so the only way to tell one from another, in a catch block or in a test, was to match the prose. The message became the contract by accident, and a test asserting it breaks on a reword and holds when the behaviour is wrong. Each now has its own exported type, over a shared base so a caller can catch the lot or one of them, following the shape build-graphql already uses. The base passes its message to Error rather than leaving it empty, since these surface in a build log where the sentence is what gets read. Also removes four test files that existed to move a coverage number rather than to prove anything. They asserted the message text of the line above them, or how many arguments reached console, which can only fail when the implementation is reworded and cannot fail when the behaviour is wrong. The logger tests were the worst of it: a console wrapper is not ours to prove, and testing it meant replacing console and asserting on that instead.
shellicar
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
One cause produced the reported bug three ways, because the matcher compared esbuild's metafile keys against
path.relativeoutput as strings: esbuild always writes forward slashes, its keys are relative to its own working directory rather than the process one, and a case-folding filesystem reports a stored name that need not match what esbuild wrote. Matching by filesystem identity settles all three. The report came from Windows, but macOS was affected too.Refusing to clean is a behaviour change rather than a fix: where the plugin cannot identify anything the build produced, it now removes nothing and says why. That is deliberately not a build failure, so an upgrade cannot start breaking builds, and the new
strictoption turns it into one for anyone who wants that.build-clean had no tests. It has them now, and they were pushed failing first so each defect was demonstrated before anything changed.