From b2678bb79278ffebae725b0172a9952e1ccee041 Mon Sep 17 00:00:00 2001 From: Alan Agius Date: Thu, 12 Oct 2023 11:50:52 +0000 Subject: [PATCH] fix(@angular-devkit/build-angular): several fixes to assets and files writes in browser-esbuild builder This commit ports https://github.com/angular/angular-cli/pull/26016 to the esbuilder and also fixes an issue where assets were being outputted in the wrong directory. Closes #26021 --- goldens/circular-deps/packages.json | 4 + .../src/builders/browser-esbuild/index.ts | 53 +-- .../tests/options/assets_spec.ts | 380 ++++++++++++++++++ .../build_angular/src/tools/esbuild/utils.ts | 74 ++-- 4 files changed, 445 insertions(+), 66 deletions(-) create mode 100644 packages/angular_devkit/build_angular/src/builders/browser-esbuild/tests/options/assets_spec.ts diff --git a/goldens/circular-deps/packages.json b/goldens/circular-deps/packages.json index 1af91b15b564..e7fdcce2f8ca 100644 --- a/goldens/circular-deps/packages.json +++ b/goldens/circular-deps/packages.json @@ -3,6 +3,10 @@ "packages/angular_devkit/build_angular/src/tools/esbuild/bundler-context.ts", "packages/angular_devkit/build_angular/src/tools/esbuild/utils.ts" ], + [ + "packages/angular_devkit/build_angular/src/tools/esbuild/bundler-execution-result.ts", + "packages/angular_devkit/build_angular/src/tools/esbuild/utils.ts" + ], [ "packages/angular_devkit/build_angular/src/tools/webpack/utils/stats.ts", "packages/angular_devkit/build_angular/src/utils/bundle-calculator.ts" diff --git a/packages/angular_devkit/build_angular/src/builders/browser-esbuild/index.ts b/packages/angular_devkit/build_angular/src/builders/browser-esbuild/index.ts index e1e3f243be53..cb17074048b7 100644 --- a/packages/angular_devkit/build_angular/src/builders/browser-esbuild/index.ts +++ b/packages/angular_devkit/build_angular/src/builders/browser-esbuild/index.ts @@ -12,6 +12,8 @@ import { constants as fsConstants } from 'node:fs'; import fs from 'node:fs/promises'; import path from 'node:path'; import { BuildOutputFile } from '../../tools/esbuild/bundler-context'; +import { BuildOutputAsset } from '../../tools/esbuild/bundler-execution-result'; +import { emitFilesToDisk } from '../../tools/esbuild/utils'; import { buildApplicationInternal } from '../application'; import { Schema as ApplicationBuilderOptions } from '../application/schema'; import { logBuilderStatusWarnings } from './builder-status-warnings'; @@ -74,36 +76,37 @@ function normalizeOptions(options: BrowserBuilderOptions): ApplicationBuilderOpt // and not output browser files into '/browser'. async function writeResultFiles( outputFiles: BuildOutputFile[], - assetFiles: { source: string; destination: string }[] | undefined, + assetFiles: BuildOutputAsset[] | undefined, outputPath: string, ) { const directoryExists = new Set(); - await Promise.all( - outputFiles.map(async (file) => { - // Ensure output subdirectories exist - const basePath = path.dirname(file.path); - if (basePath && !directoryExists.has(basePath)) { - await fs.mkdir(path.join(outputPath, basePath), { recursive: true }); - directoryExists.add(basePath); - } - // Write file contents - await fs.writeFile(path.join(outputPath, file.path), file.contents); - }), - ); + const ensureDirectoryExists = async (basePath: string) => { + if (basePath && !directoryExists.has(basePath)) { + await fs.mkdir(path.join(outputPath, basePath), { recursive: true }); + directoryExists.add(basePath); + } + }; + + // Writes the output file to disk and ensures the containing directories are present + await emitFilesToDisk(outputFiles, async (file: BuildOutputFile) => { + // Ensure output subdirectories exist + const basePath = path.dirname(file.path); + await ensureDirectoryExists(basePath); + + // Write file contents + await fs.writeFile(path.join(outputPath, file.path), file.contents); + }); if (assetFiles?.length) { - await Promise.all( - assetFiles.map(async ({ source, destination }) => { - // Ensure output subdirectories exist - const basePath = path.dirname(destination); - if (basePath && !directoryExists.has(basePath)) { - await fs.mkdir(path.join(outputPath, basePath), { recursive: true }); - directoryExists.add(basePath); - } - // Copy file contents - await fs.copyFile(source, path.join(outputPath), fsConstants.COPYFILE_FICLONE); - }), - ); + await emitFilesToDisk(assetFiles, async ({ source, destination }) => { + const basePath = path.dirname(destination); + + // Ensure output subdirectories exist + await ensureDirectoryExists(basePath); + + // Copy file contents + await fs.copyFile(source, path.join(outputPath, destination), fsConstants.COPYFILE_FICLONE); + }); } } diff --git a/packages/angular_devkit/build_angular/src/builders/browser-esbuild/tests/options/assets_spec.ts b/packages/angular_devkit/build_angular/src/builders/browser-esbuild/tests/options/assets_spec.ts new file mode 100644 index 000000000000..26482b8f3998 --- /dev/null +++ b/packages/angular_devkit/build_angular/src/builders/browser-esbuild/tests/options/assets_spec.ts @@ -0,0 +1,380 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import { buildEsbuildBrowser } from '../../index'; +import { BASE_OPTIONS, BROWSER_BUILDER_INFO, describeBuilder } from '../setup'; + +describeBuilder(buildEsbuildBrowser, BROWSER_BUILDER_INFO, (harness) => { + describe('Option: "assets"', () => { + beforeEach(async () => { + // Application code is not needed for asset tests + await harness.writeFile('src/main.ts', 'console.log("TEST");'); + }); + + it('supports an empty array value', async () => { + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: [], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + }); + + it('supports mixing shorthand and longhand syntax', async () => { + await harness.writeFile('src/files/test.svg', ''); + await harness.writeFile('src/files/another.file', 'asset file'); + await harness.writeFile('src/extra.file', 'extra file'); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: ['src/extra.file', { glob: '*', input: 'src/files', output: '.' }], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/extra.file').content.toBe('extra file'); + harness.expectFile('dist/test.svg').content.toBe(''); + harness.expectFile('dist/another.file').content.toBe('asset file'); + }); + + describe('shorthand syntax', () => { + it('copies a single asset', async () => { + await harness.writeFile('src/test.svg', ''); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: ['src/test.svg'], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/test.svg').content.toBe(''); + }); + + it('copies multiple assets', async () => { + await harness.writeFile('src/test.svg', ''); + await harness.writeFile('src/another.file', 'asset file'); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: ['src/test.svg', 'src/another.file'], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/test.svg').content.toBe(''); + harness.expectFile('dist/another.file').content.toBe('asset file'); + }); + + it('copies an asset with directory and maintains directory in output', async () => { + await harness.writeFile('src/subdirectory/test.svg', ''); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: ['src/subdirectory/test.svg'], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/subdirectory/test.svg').content.toBe(''); + }); + + it('does not fail if asset does not exist', async () => { + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: ['src/test.svg'], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/test.svg').toNotExist(); + }); + + it('fail if asset path is not within project source root', async () => { + await harness.writeFile('test.svg', ''); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: ['test.svg'], + }); + + const { error } = await harness.executeOnce({ outputLogsOnException: false }); + + expect(error?.message).toMatch('path must start with the project source root'); + + harness.expectFile('dist/test.svg').toNotExist(); + }); + }); + + describe('longhand syntax', () => { + it('copies a single asset', async () => { + await harness.writeFile('src/test.svg', ''); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: [{ glob: 'test.svg', input: 'src', output: '.' }], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/test.svg').content.toBe(''); + }); + + it('copies multiple assets as separate entries', async () => { + await harness.writeFile('src/test.svg', ''); + await harness.writeFile('src/another.file', 'asset file'); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: [ + { glob: 'test.svg', input: 'src', output: '.' }, + { glob: 'another.file', input: 'src', output: '.' }, + ], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/test.svg').content.toBe(''); + harness.expectFile('dist/another.file').content.toBe('asset file'); + }); + + it('copies multiple assets with a single entry glob pattern', async () => { + await harness.writeFile('src/test.svg', ''); + await harness.writeFile('src/another.file', 'asset file'); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: [{ glob: '{test.svg,another.file}', input: 'src', output: '.' }], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/test.svg').content.toBe(''); + harness.expectFile('dist/another.file').content.toBe('asset file'); + }); + + it('copies multiple assets with a wildcard glob pattern', async () => { + await harness.writeFile('src/files/test.svg', ''); + await harness.writeFile('src/files/another.file', 'asset file'); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: [{ glob: '*', input: 'src/files', output: '.' }], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/test.svg').content.toBe(''); + harness.expectFile('dist/another.file').content.toBe('asset file'); + }); + + it('copies multiple assets with a recursive wildcard glob pattern', async () => { + await harness.writeFiles({ + 'src/files/test.svg': '', + 'src/files/another.file': 'asset file', + 'src/files/nested/extra.file': 'extra file', + }); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: [{ glob: '**/*', input: 'src/files', output: '.' }], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/test.svg').content.toBe(''); + harness.expectFile('dist/another.file').content.toBe('asset file'); + harness.expectFile('dist/nested/extra.file').content.toBe('extra file'); + }); + + it('automatically ignores "." prefixed files when using wildcard glob pattern', async () => { + await harness.writeFile('src/files/.gitkeep', ''); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: [{ glob: '*', input: 'src/files', output: '.' }], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/.gitkeep').toNotExist(); + }); + + it('supports ignoring a specific file when using a glob pattern', async () => { + await harness.writeFiles({ + 'src/files/test.svg': '', + 'src/files/another.file': 'asset file', + 'src/files/nested/extra.file': 'extra file', + }); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: [{ glob: '**/*', input: 'src/files', output: '.', ignore: ['another.file'] }], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/test.svg').content.toBe(''); + harness.expectFile('dist/another.file').toNotExist(); + harness.expectFile('dist/nested/extra.file').content.toBe('extra file'); + }); + + it('supports ignoring with a glob pattern when using a glob pattern', async () => { + await harness.writeFiles({ + 'src/files/test.svg': '', + 'src/files/another.file': 'asset file', + 'src/files/nested/extra.file': 'extra file', + }); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: [{ glob: '**/*', input: 'src/files', output: '.', ignore: ['**/*.file'] }], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/test.svg').content.toBe(''); + harness.expectFile('dist/another.file').toNotExist(); + harness.expectFile('dist/nested/extra.file').toNotExist(); + }); + + it('copies an asset with directory and maintains directory in output', async () => { + await harness.writeFile('src/subdirectory/test.svg', ''); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: [{ glob: 'subdirectory/test.svg', input: 'src', output: '.' }], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/subdirectory/test.svg').content.toBe(''); + }); + + it('does not fail if asset does not exist', async () => { + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: [{ glob: 'test.svg', input: 'src', output: '.' }], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/test.svg').toNotExist(); + }); + + it('uses project output path when output option is empty string', async () => { + await harness.writeFile('src/test.svg', ''); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: [{ glob: 'test.svg', input: 'src', output: '' }], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/test.svg').content.toBe(''); + }); + + it('uses project output path when output option is "."', async () => { + await harness.writeFile('src/test.svg', ''); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: [{ glob: 'test.svg', input: 'src', output: '.' }], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/test.svg').content.toBe(''); + }); + + it('uses project output path when output option is "/"', async () => { + await harness.writeFile('src/test.svg', ''); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: [{ glob: 'test.svg', input: 'src', output: '/' }], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/test.svg').content.toBe(''); + }); + + it('creates a project output sub-path when output option path does not exist', async () => { + await harness.writeFile('src/test.svg', ''); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: [{ glob: 'test.svg', input: 'src', output: 'subdirectory' }], + }); + + const { result } = await harness.executeOnce(); + + expect(result?.success).toBe(true); + + harness.expectFile('dist/subdirectory/test.svg').content.toBe(''); + }); + + it('fails if output option is not within project output path', async () => { + await harness.writeFile('test.svg', ''); + + harness.useTarget('build', { + ...BASE_OPTIONS, + assets: [{ glob: 'test.svg', input: 'src', output: '..' }], + }); + + const { error } = await harness.executeOnce({ outputLogsOnException: false }); + + expect(error?.message).toMatch( + 'An asset cannot be written to a location outside of the output path', + ); + + harness.expectFile('dist/test.svg').toNotExist(); + }); + }); + }); +}); diff --git a/packages/angular_devkit/build_angular/src/tools/esbuild/utils.ts b/packages/angular_devkit/build_angular/src/tools/esbuild/utils.ts index 4f63f44ef081..d18fc16fb846 100644 --- a/packages/angular_devkit/build_angular/src/tools/esbuild/utils.ts +++ b/packages/angular_devkit/build_angular/src/tools/esbuild/utils.ts @@ -19,6 +19,7 @@ import { BudgetCalculatorResult } from '../../utils/bundle-calculator'; import { Spinner } from '../../utils/spinner'; import { BundleStats, generateBuildStatsTable } from '../webpack/utils/stats'; import { BuildOutputFile, BuildOutputFileType, InitialFileRecord } from './bundler-context'; +import { BuildOutputAsset } from './bundler-execution-result'; const compressAsync = promisify(brotliCompress); @@ -175,68 +176,59 @@ export function getFeatureSupport(target: string[]): BuildOptions['supported'] { return supported; } -const MAX_CONCURRENT_WRITES = 64; - export async function writeResultFiles( outputFiles: BuildOutputFile[], - assetFiles: { source: string; destination: string }[] | undefined, + assetFiles: BuildOutputAsset[] | undefined, outputPath: string, ) { const directoryExists = new Set(); + const ensureDirectoryExists = async (basePath: string) => { + if (basePath && !directoryExists.has(basePath)) { + await fs.mkdir(path.join(outputPath, basePath), { recursive: true }); + directoryExists.add(basePath); + } + }; // Writes the output file to disk and ensures the containing directories are present - const writeOutputFile = async (file: BuildOutputFile) => { + await emitFilesToDisk(outputFiles, async (file: BuildOutputFile) => { const fullOutputPath = file.fullOutputPath; // Ensure output subdirectories exist const basePath = path.dirname(fullOutputPath); - if (basePath && !directoryExists.has(basePath)) { - await fs.mkdir(path.join(outputPath, basePath), { recursive: true }); - directoryExists.add(basePath); - } + await ensureDirectoryExists(basePath); + // Write file contents await fs.writeFile(path.join(outputPath, fullOutputPath), file.contents); - }; + }); + + if (assetFiles?.length) { + await emitFilesToDisk(assetFiles, async ({ source, destination }) => { + // Ensure output subdirectories exist + const destPath = join('browser', destination); + const basePath = path.dirname(destPath); + await ensureDirectoryExists(basePath); + // Copy file contents + await fs.copyFile(source, path.join(outputPath, destPath), fsConstants.COPYFILE_FICLONE); + }); + } +} + +const MAX_CONCURRENT_WRITES = 64; +export async function emitFilesToDisk( + files: T[], + writeFileCallback: (file: T) => Promise, +): Promise { // Write files in groups of MAX_CONCURRENT_WRITES to avoid too many open files - for (let fileIndex = 0; fileIndex < outputFiles.length; ) { - const groupMax = Math.min(fileIndex + MAX_CONCURRENT_WRITES, outputFiles.length); + for (let fileIndex = 0; fileIndex < files.length; ) { + const groupMax = Math.min(fileIndex + MAX_CONCURRENT_WRITES, files.length); const actions = []; while (fileIndex < groupMax) { - actions.push(writeOutputFile(outputFiles[fileIndex++])); + actions.push(writeFileCallback(files[fileIndex++])); } await Promise.all(actions); } - - if (assetFiles?.length) { - const copyAssetFile = async (asset: { source: string; destination: string }) => { - // Ensure output subdirectories exist - const destPath = join('browser', asset.destination); - const basePath = path.dirname(destPath); - if (basePath && !directoryExists.has(basePath)) { - await fs.mkdir(path.join(outputPath, basePath), { recursive: true }); - directoryExists.add(basePath); - } - // Copy file contents - await fs.copyFile( - asset.source, - path.join(outputPath, destPath), - fsConstants.COPYFILE_FICLONE, - ); - }; - - for (let fileIndex = 0; fileIndex < assetFiles.length; ) { - const groupMax = Math.min(fileIndex + MAX_CONCURRENT_WRITES, assetFiles.length); - - const actions = []; - while (fileIndex < groupMax) { - actions.push(copyAssetFile(assetFiles[fileIndex++])); - } - - await Promise.all(actions); - } - } } export function createOutputFileFromText(