-
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
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
421684b
Refactor the bundler-report process to be more predictive
yoannmoinet d1d4698
Add more tests to bundler-report
yoannmoinet 44bea9a
Add more tests to sourcemaps uploading
yoannmoinet 7e37ce5
Integrity
yoannmoinet a22a62b
Re-use existing helper
yoannmoinet 7bd30be
Fix no root vite
yoannmoinet e7835a3
Merge branch 'master' into yoann/fix-vite-outdir-sourcemaps
yoannmoinet c6465c9
Move vite plugin in its own function.
yoannmoinet 28a9539
Be more explicit with what's happening
yoannmoinet b297c24
Change getOutDirFromOutputs logic to simplify readability
yoannmoinet c8a6c27
Iso changes
yoannmoinet File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ | |
"@dd/core": "workspace:*" | ||
}, | ||
"devDependencies": { | ||
"rollup": "4.24.2", | ||
"typescript": "5.4.3" | ||
} | ||
} |
168 changes: 168 additions & 0 deletions
168
packages/plugins/bundler-report/src/helpers/rollup.test.ts
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
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,168 @@ | ||||||
// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. | ||||||
// This product includes software developed at Datadog (https://www.datadoghq.com/). | ||||||
// Copyright 2019-Present Datadog, Inc. | ||||||
|
||||||
import { addFixtureFiles } from '@dd/tests/_jest/helpers/mocks'; | ||||||
import type { InputOptions } from 'rollup'; | ||||||
|
||||||
import { computeCwd, getOutDirFromOutputs } from './rollup'; | ||||||
|
||||||
jest.mock('@dd/core/helpers/fs', () => { | ||||||
const original = jest.requireActual('@dd/core/helpers/fs'); | ||||||
return { | ||||||
...original, | ||||||
existsSync: jest.fn(), | ||||||
}; | ||||||
}); | ||||||
|
||||||
describe('Rollup Helpers', () => { | ||||||
describe('getOutDirFromOutputs', () => { | ||||||
const cases = [ | ||||||
{ | ||||||
description: 'extract dir from single output object with dir', | ||||||
outputOptions: { dir: 'dist' }, | ||||||
expected: 'dist', | ||||||
}, | ||||||
{ | ||||||
description: 'extract dir from single output object with file', | ||||||
outputOptions: { file: 'dist/bundle.js' }, | ||||||
expected: 'dist', | ||||||
}, | ||||||
{ | ||||||
description: 'extract dir from array of outputs with dir', | ||||||
outputOptions: [{ dir: 'dist' }, { dir: 'dist2' }], | ||||||
expected: 'dist', | ||||||
}, | ||||||
{ | ||||||
description: 'extract dir from array of outputs with file', | ||||||
outputOptions: [{ file: 'dist/bundle.js' }, { file: 'dist2/bundle.js' }], | ||||||
expected: 'dist', | ||||||
}, | ||||||
{ | ||||||
description: 'prefer dir over file in same output', | ||||||
outputOptions: { dir: 'dist', file: 'other/bundle.js' }, | ||||||
expected: 'dist', | ||||||
}, | ||||||
{ | ||||||
description: 'handle nested file paths', | ||||||
outputOptions: { file: 'dist/assets/js/bundle.js' }, | ||||||
expected: 'dist/assets/js', | ||||||
}, | ||||||
{ | ||||||
description: 'return undefined for no outputOptions', | ||||||
outputOptions: undefined, | ||||||
expected: undefined, | ||||||
}, | ||||||
{ | ||||||
description: 'return undefined for empty array', | ||||||
outputOptions: [], | ||||||
expected: undefined, | ||||||
}, | ||||||
{ | ||||||
description: 'return undefined when no dir or file specified', | ||||||
outputOptions: [{ format: 'esm' }, { format: 'cjs' }], | ||||||
expected: undefined, | ||||||
}, | ||||||
]; | ||||||
|
||||||
test.each(cases)('Should $description', ({ outputOptions, expected }) => { | ||||||
// Rollup doesn't type InputOptions['output'], yet it exists. | ||||||
expect(getOutDirFromOutputs({ output: outputOptions } as InputOptions)).toBe(expected); | ||||||
}); | ||||||
}); | ||||||
|
||||||
describe('computeCwd', () => { | ||||||
beforeAll(() => { | ||||||
jest.spyOn(process, 'cwd').mockReturnValue('/base/cwd'); | ||||||
}); | ||||||
|
||||||
beforeEach(() => { | ||||||
// Set up virtual file system for package.json files | ||||||
addFixtureFiles({ | ||||||
'/project/package.json': '', | ||||||
'/project/src/package.json': '', | ||||||
'/project/lib/package.json': '', | ||||||
'/base/cwd/package.json': '', | ||||||
}); | ||||||
}); | ||||||
|
||||||
const cases = [ | ||||||
{ | ||||||
description: 'handle string input', | ||||||
options: { input: '/project/src/index.js' }, | ||||||
expected: '/project', | ||||||
}, | ||||||
{ | ||||||
description: 'handle array input', | ||||||
options: { | ||||||
input: ['/project/src/index.js', '/project/lib/util.js'], | ||||||
}, | ||||||
expected: '/project', | ||||||
}, | ||||||
{ | ||||||
description: 'handle object input', | ||||||
options: { | ||||||
input: { | ||||||
main: '/project/src/index.js', | ||||||
util: '/project/lib/util.js', | ||||||
}, | ||||||
}, | ||||||
expected: '/project', | ||||||
}, | ||||||
{ | ||||||
description: 'throw error for invalid input type in object', | ||||||
options: { input: { main: 123 } }, | ||||||
shouldThrow: 'Invalid input type', | ||||||
}, | ||||||
{ | ||||||
description: 'throw error for invalid input type in array', | ||||||
options: { input: [123] }, | ||||||
shouldThrow: 'Invalid input type', | ||||||
}, | ||||||
{ | ||||||
description: 'include absolute output directory in cwd computation', | ||||||
options: { | ||||||
input: '/project/src/index.js', | ||||||
output: { dir: '/project/dist' }, | ||||||
}, | ||||||
expected: '/project', | ||||||
}, | ||||||
{ | ||||||
description: 'ignore relative output directory', | ||||||
options: { | ||||||
input: '/project/src/index.js', | ||||||
output: { dir: 'dist' }, | ||||||
}, | ||||||
expected: '/project', | ||||||
}, | ||||||
{ | ||||||
description: 'fallback to process.cwd when no input', | ||||||
options: {}, | ||||||
expected: '/base/cwd', | ||||||
}, | ||||||
{ | ||||||
description: 'fallback to process.cwd with relative input', | ||||||
options: { input: 'index.js' }, | ||||||
expected: '/base/cwd', | ||||||
}, | ||||||
]; | ||||||
|
||||||
test.each(cases)('Should $description', ({ options, expected, shouldThrow }) => { | ||||||
const errors = []; | ||||||
const results = []; | ||||||
const expectedResults = expected ? [expected] : []; | ||||||
const expectedErrors = shouldThrow ? [shouldThrow] : []; | ||||||
|
||||||
try { | ||||||
// Rollup doesn't type InputOptions['output'], yet it exists. | ||||||
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 commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TypeScript won't let me.
|
||||||
errors.push(error.message); | ||||||
} | ||||||
|
||||||
expect(errors).toEqual(expectedErrors); | ||||||
expect(results).toEqual(expectedResults); | ||||||
}); | ||||||
}); | ||||||
}); |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
// Unless explicitly stated otherwise all files in this repository are licensed under the MIT License. | ||
// This product includes software developed at Datadog (https://www.datadoghq.com/). | ||
// Copyright 2019-Present Datadog, Inc. | ||
|
||
import { getHighestPackageJsonDir, getNearestCommonDirectory } from '@dd/core/helpers/paths'; | ||
import path from 'path'; | ||
import type { InputOptions } from 'rollup'; | ||
|
||
// Compute the CWD based on a list of directories. | ||
const getCwd = (dirs: Set<string>) => { | ||
yoannmoinet marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const dirsToUse: Set<string> = new Set(dirs); | ||
for (const dir of dirs) { | ||
const highestPackage = getHighestPackageJsonDir(dir); | ||
if (highestPackage && !dirs.has(highestPackage)) { | ||
dirsToUse.add(highestPackage); | ||
} | ||
} | ||
|
||
// Fall back to the nearest common directory. | ||
const nearestDir = getNearestCommonDirectory(Array.from(dirsToUse)); | ||
if (nearestDir === path.sep) { | ||
return undefined; | ||
} | ||
return nearestDir; | ||
}; | ||
|
||
export const getOutDirFromOutputs = (options: InputOptions) => { | ||
const hasOutput = 'output' in options && options.output; | ||
if (!hasOutput) { | ||
return undefined; | ||
} | ||
|
||
const outputOptions = options.output; | ||
const normalizedOutputOptions = Array.isArray(outputOptions) ? outputOptions : [outputOptions]; | ||
|
||
// FIXME: This is an oversimplification, we should handle builds with multiple outputs. | ||
// Ideally, `outDir` should only be computed for the build-report. | ||
// And build-report should also handle multiple outputs. | ||
for (const output of normalizedOutputOptions) { | ||
if (output.dir) { | ||
return output.dir; | ||
} | ||
if (output.file) { | ||
return path.dirname(output.file); | ||
} | ||
} | ||
}; | ||
|
||
export const computeCwd = (options: InputOptions) => { | ||
const directoriesForCwd: Set<string> = new Set(); | ||
|
||
if (options.input) { | ||
const normalizedInput = Array.isArray(options.input) | ||
? options.input | ||
: typeof options.input === 'object' | ||
? Object.values(options.input) | ||
: [options.input]; | ||
|
||
for (const input of normalizedInput) { | ||
if (typeof input !== 'string') { | ||
throw new Error('Invalid input type'); | ||
} | ||
directoriesForCwd.add(path.dirname(input)); | ||
} | ||
} | ||
|
||
// In case an absolute path has been provided in the output options, | ||
// we include it in the directories list for CWD computation. | ||
const outDirFromOutputs = getOutDirFromOutputs(options); | ||
if (outDirFromOutputs && path.isAbsolute(outDirFromOutputs)) { | ||
directoriesForCwd.add(outDirFromOutputs); | ||
} | ||
|
||
const cwd = getCwd(directoriesForCwd); | ||
|
||
if (cwd) { | ||
return cwd; | ||
} | ||
|
||
// Fallback to process.cwd() as would Vite and Rollup do in their own process. | ||
return process.cwd(); | ||
}; |
Oops, something went wrong.
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.
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:
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.