-
Notifications
You must be signed in to change notification settings - Fork 8
[fix] Vite outDir for sourcemaps upload (take 2) #186
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
Conversation
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.
Pull Request Overview
This PR ensures bundler output directories are normalized to absolute paths for reliable sourcemaps uploading and refactors the bundler-report plugin to leverage new helper functions.
- Normalize
decomposePath
API and update sourcemaps tests to use absoluteoutDir
- Refactor bundler-report plugin (
index.ts
) to usecomputeCwd
,computeOutDir
, and related helpers - Introduce
rollup.ts
helpers and expand tests; addrollup
as a devDependency
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/plugins/error-tracking/src/sourcemaps/index.test.ts | Expanded scenario tests covering multiple bundlers and prefixes |
packages/plugins/error-tracking/src/sourcemaps/files.ts | Updated decomposePath to accept prefix and absolute outDir |
packages/plugins/error-tracking/src/sourcemaps/files.test.ts | Adjusted tests to match new decomposePath signature |
packages/plugins/bundler-report/src/index.ts | Refactored plugin logic to use rollup helpers for path/CWD |
packages/plugins/bundler-report/src/index.test.ts | Rewrote tests to validate outDir and cwd per bundler |
packages/plugins/bundler-report/src/helpers/rollup.ts | Added computeCwd , computeOutDir , etc., for rollup support |
packages/plugins/bundler-report/src/helpers/rollup.test.ts | Added tests covering rollup helper functions |
packages/plugins/bundler-report/package.json | Added rollup devDependency |
Comments suppressed due to low confidence (1)
packages/plugins/bundler-report/src/index.test.ts:361
- The mock functions
reportCalls
andcwdCalls
are not reset between test cases, causing cumulative counts. Add abeforeEach
or invoke.mockClear()
to reset them before each test.
expect(reportCalls).toHaveBeenCalledTimes(1);
🚨 BugBot couldn't runBugBot is experiencing high demand right now. Try again in a few minutes by commenting "bugbot run" (requestId: serverGenReqId_bcbd29d0-616a-45d5-8e7b-79cb9ba9048c). |
bugbot run |
🚨 BugBot couldn't runBugBot is experiencing high demand right now. Try again in a few minutes by commenting "bugbot run" (requestId: serverGenReqId_db6684b4-6f19-4bc5-8340-8133e25f3a39). |
bugbot run |
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.
Bug: Shared State Causes Incorrect CWD and OutDir
The gotViteCwd
variable is declared at module scope, creating shared state across plugin instances. This causes two issues in Vite plugin hooks:
- Incorrect CWD determination: In watch mode or concurrent builds, the flag retains state or causes race conditions, preventing
context.cwd
from being correctly reset toprocess.cwd()
whenconfig.root
is not provided. - Incorrect
outDir
calculation: Whenconfig.root
is not provided andconfig.build.outDir
is an absolute path, thepath.relative
calculation in theoptions
hook uses a stalecontext.cwd
(before it's updated toprocess.cwd()
), resulting in an incorrectly computedcontext.bundler.outDir
.
packages/plugins/bundler-report/src/index.ts#L42-L131
build-plugins/packages/plugins/bundler-report/src/index.ts
Lines 42 to 131 in e7835a3
const { context } = arg; | |
let gotViteCwd = false; | |
const bundlerReportPlugin: PluginOptions = { | |
name: PLUGIN_NAME, | |
enforce: 'pre', | |
esbuild: { | |
setup(build) { | |
context.bundler.rawConfig = build.initialOptions; | |
if (build.initialOptions.absWorkingDir) { | |
context.cwd = build.initialOptions.absWorkingDir; | |
} | |
if (build.initialOptions.outdir) { | |
context.bundler.outDir = getAbsoluteOutDir( | |
context.cwd, | |
build.initialOptions.outdir, | |
); | |
} | |
if (build.initialOptions.outfile) { | |
context.bundler.outDir = getAbsoluteOutDir( | |
context.cwd, | |
path.dirname(build.initialOptions.outfile), | |
); | |
} | |
context.hook('cwd', context.cwd); | |
context.hook('bundlerReport', context.bundler); | |
// We force esbuild to produce its metafile. | |
build.initialOptions.metafile = true; | |
}, | |
}, | |
webpack: xpackPlugin(context), | |
rspack: xpackPlugin(context), | |
// Vite and Rollup have (almost) the same API. | |
// They don't really support the CWD concept, | |
// so we have to compute it based on existing configurations. | |
// The basic idea is to compare input vs output and keep the common part of the paths. | |
vite: { | |
config(config) { | |
context.bundler.rawConfig = config; | |
let outDir = ''; | |
// If we have the outDir configuration from Vite. | |
if (config.build?.outDir) { | |
outDir = config.build.outDir; | |
} else { | |
outDir = 'dist'; | |
} | |
if (config.root) { | |
context.cwd = config.root; | |
context.hook('cwd', context.cwd); | |
gotViteCwd = true; | |
} | |
// Make sure the outDir is absolute. | |
context.bundler.outDir = getAbsoluteOutDir(context.cwd, outDir); | |
}, | |
options(options) { | |
// If we couldn't set the CWD in the config hook, we fallback here. | |
if (!gotViteCwd) { | |
// Reset the CWD/outDir from the config hook. | |
const relativeOutDir = path.relative(context.cwd, context.bundler.outDir); | |
// Vite will fallback to process.cwd() if no root is provided. | |
context.cwd = process.cwd(); | |
context.hook('cwd', context.cwd); | |
// Update the bundler's outDir based on the CWD. | |
context.bundler.outDir = getAbsoluteOutDir(context.cwd, relativeOutDir); | |
} | |
// When output is provided, rollup will take over and ignore vite's outDir. | |
if ('output' in options) { | |
// When you use `rollupOptions.output.dir` in Vite, | |
// the absolute path for outDir is computed based on the process' CWD. | |
const outDir = getAbsoluteOutDir( | |
process.cwd(), | |
getOutDirFromOutputs(options.output as OutputOptions), | |
); | |
if (outDir) { | |
context.bundler.outDir = outDir; | |
} | |
} | |
context.hook('bundlerReport', context.bundler); | |
}, |
Was this report helpful? Give feedback by reacting with 👍 or 👎
const expectedErrors = shouldThrow ? [shouldThrow] : []; | ||
|
||
try { | ||
const result = computeCwd(options as InputOptions); |
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.
similarly here
Potentially you can add a:
const cases = [
// ...
] satisfies Array<{ description: string; expected: string; options: InputOptions }>;
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.
Still need it because Rollup doesn't type InputOptions['output']
, yet, it's there.
Added a comment to explain it.
try { | ||
const result = computeCwd(options as InputOptions); | ||
results.push(result); | ||
} catch (error: any) { |
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.
} catch (error: any) { | |
} catch (error: Error) { |
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.
TypeScript won't let me.
Catch clause variable type annotation must be 'any' or 'unknown' if specified.
ts(1196)
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.
My remaining comments are just suggestions for improvements, not required changes, so lgtm
Feel free to apply them if you want
- early outs - comments - types
context.bundler.outDir = getAbsolutePath(process.cwd(), context.bundler.outDir); | ||
// Update the bundler's outDir based on the CWD. | ||
context.bundler.outDir = getAbsoluteOutDir(context.cwd, relativeOutDir); | ||
} |
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.
Bug: Vite Plugin Fails to Adjust Output Directory
The Vite plugin's options
hook contains a bug in its fallback logic when config.root
is not provided. When context.cwd
is reset to process.cwd()
, the context.bundler.outDir
is recalculated. The issue arises because path.relative
is used to compute a relative path from the old context.cwd
to the existing context.bundler.outDir
(which might be an absolute path or an empty string). This relative path is then incorrectly resolved against the new context.cwd
(i.e., process.cwd()
), leading to an incorrect final context.bundler.outDir
.
Locations (1)
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.
What would be the vite configuration that would lead to this bug?
What and why?
Fixing #179 again, apparently #182 didn't fully fix it.
How?
I think the uncertainty and flimsiness is coming from the internal
bundler-report
wherebundler.outDir
wouldn't be totally normalised across bundlers (sometimes relative, absolute, leading slashes, ...).So I totally reworked this section to be normalised as an absolute path.
Added a ton of tests in both this section and the sourcemaps uploading.