From 60f5b604c3c20c8fae5812e3da38e97f428b4dc8 Mon Sep 17 00:00:00 2001 From: Alan Agius Date: Fri, 5 Aug 2022 13:44:24 +0000 Subject: [PATCH] feat(@angular-devkit/build-angular): switch to use Sass modern API Sass modern API provides faster compilations times when used in an async manner. |Application compilation duration | Sass API and Compiler| |-- | --| |60852ms | dart-sass legacy sync API| |52666ms | dart-sass modern API| Note: https://github.com/johannesjo/super-productivity was used for benchmarking. Prior art: http://docs/document/d/1CvEceWMpBoEBd8SfvksGMdVHxaZMH93b0EGS3XbR3_Q?resourcekey=0-vFm-xMspT65FZLIyX7xWFQ BREAKING CHANGE: Deprecated support for tilde import has been removed. Please update the imports by removing the `~`. Before ```scss @import "~font-awesome/scss/font-awesome"; ``` After ```scss @import "font-awesome/scss/font-awesome"; ``` --- .../src/builders/browser/specs/styles_spec.ts | 24 +-- .../build_angular/src/sass/sass-service.ts | 172 ++++++++++-------- .../build_angular/src/sass/worker.ts | 77 ++++++-- .../src/webpack/configs/styles.ts | 93 ++++++---- 4 files changed, 213 insertions(+), 153 deletions(-) diff --git a/packages/angular_devkit/build_angular/src/builders/browser/specs/styles_spec.ts b/packages/angular_devkit/build_angular/src/builders/browser/specs/styles_spec.ts index 08c4fb6c4673..a037cd0bde7e 100644 --- a/packages/angular_devkit/build_angular/src/builders/browser/specs/styles_spec.ts +++ b/packages/angular_devkit/build_angular/src/builders/browser/specs/styles_spec.ts @@ -260,19 +260,14 @@ describe('Browser Builder styles', () => { }); }); - /** - * font-awesome mock to avoid having an extra dependency. - */ - function mockFontAwesomePackage(host: TestProjectHost): void { + it(`supports font-awesome imports`, async () => { + // font-awesome mock to avoid having an extra dependency. host.writeMultipleFiles({ 'node_modules/font-awesome/scss/font-awesome.scss': ` - * { color: red } + * { color: red } `, }); - } - it(`supports font-awesome imports`, async () => { - mockFontAwesomePackage(host); host.writeMultipleFiles({ 'src/styles.scss': ` @import "font-awesome/scss/font-awesome"; @@ -283,19 +278,6 @@ describe('Browser Builder styles', () => { await browserBuild(architect, host, target, overrides); }); - it(`supports font-awesome imports (tilde)`, async () => { - mockFontAwesomePackage(host); - host.writeMultipleFiles({ - 'src/styles.scss': ` - $fa-font-path: "~font-awesome/fonts"; - @import "~font-awesome/scss/font-awesome"; - `, - }); - - const overrides = { styles: [`src/styles.scss`] }; - await browserBuild(architect, host, target, overrides); - }); - it(`uses autoprefixer`, async () => { host.writeMultipleFiles({ 'src/styles.css': tags.stripIndents` diff --git a/packages/angular_devkit/build_angular/src/sass/sass-service.ts b/packages/angular_devkit/build_angular/src/sass/sass-service.ts index 4a40334412d8..515c3910a5e3 100644 --- a/packages/angular_devkit/build_angular/src/sass/sass-service.ts +++ b/packages/angular_devkit/build_angular/src/sass/sass-service.ts @@ -7,14 +7,14 @@ */ import { - LegacyAsyncImporter as AsyncImporter, - LegacyResult as CompileResult, - LegacyException as Exception, - LegacyImporterResult as ImporterResult, - LegacyImporterThis as ImporterThis, - LegacyOptions as Options, - LegacySyncImporter as SyncImporter, + CompileResult, + Exception, + FileImporter, + Importer, + StringOptionsWithImporter, + StringOptionsWithoutImporter, } from 'sass'; +import { fileURLToPath, pathToFileURL } from 'url'; import { MessageChannel, Worker } from 'worker_threads'; import { maxWorkers } from '../utils/environment-options'; @@ -28,6 +28,8 @@ const MAX_RENDER_WORKERS = maxWorkers; */ type RenderCallback = (error?: Exception, result?: CompileResult) => void; +type FileImporterOptions = Parameters[1]; + /** * An object containing the contextual information for a specific render request. */ @@ -35,16 +37,25 @@ interface RenderRequest { id: number; workerIndex: number; callback: RenderCallback; - importers?: (SyncImporter | AsyncImporter)[]; + importers?: Importers[]; } +/** + * All available importer types. + */ +type Importers = + | Importer<'sync'> + | Importer<'async'> + | FileImporter<'sync'> + | FileImporter<'async'>; + /** * A response from the Sass render Worker containing the result of the operation. */ interface RenderResponseMessage { id: number; error?: Exception; - result?: CompileResult; + result?: Omit & { loadedUrls: string[] }; } /** @@ -71,46 +82,77 @@ export class SassWorkerImplementation { /** * The synchronous render function is not used by the `sass-loader`. */ - renderSync(): never { - throw new Error('Sass renderSync is not supported.'); + compileString(): never { + throw new Error('Sass compileString is not supported.'); } /** * Asynchronously request a Sass stylesheet to be renderered. * + * @param source The contents to compile. * @param options The `dart-sass` options to use when rendering the stylesheet. - * @param callback The function to execute when the rendering is complete. */ - render(options: Options<'async'>, callback: RenderCallback): void { + compileStringAsync( + source: string, + options: StringOptionsWithImporter<'async'> | StringOptionsWithoutImporter<'async'>, + ): Promise { // The `functions`, `logger` and `importer` options are JavaScript functions that cannot be transferred. // If any additional function options are added in the future, they must be excluded as well. - const { functions, importer, logger, ...serializableOptions } = options; + const { functions, importers, url, logger, ...serializableOptions } = options; // The CLI's configuration does not use or expose the ability to defined custom Sass functions if (functions && Object.keys(functions).length > 0) { throw new Error('Sass custom functions are not supported.'); } - let workerIndex = this.availableWorkers.pop(); - if (workerIndex === undefined) { - if (this.workers.length < MAX_RENDER_WORKERS) { - workerIndex = this.workers.length; - this.workers.push(this.createWorker()); - } else { - workerIndex = this.nextWorkerIndex++; - if (this.nextWorkerIndex >= this.workers.length) { - this.nextWorkerIndex = 0; + return new Promise((resolve, reject) => { + let workerIndex = this.availableWorkers.pop(); + if (workerIndex === undefined) { + if (this.workers.length < MAX_RENDER_WORKERS) { + workerIndex = this.workers.length; + this.workers.push(this.createWorker()); + } else { + workerIndex = this.nextWorkerIndex++; + if (this.nextWorkerIndex >= this.workers.length) { + this.nextWorkerIndex = 0; + } } } - } - const request = this.createRequest(workerIndex, callback, importer); - this.requests.set(request.id, request); + const callback: RenderCallback = (error, result) => { + if (error) { + const url = error?.span.url as string | undefined; + if (url) { + error.span.url = pathToFileURL(url); + } - this.workers[workerIndex].postMessage({ - id: request.id, - hasImporter: !!importer, - options: serializableOptions, + reject(error); + + return; + } + + if (!result) { + reject('No result.'); + + return; + } + + resolve(result); + }; + + const request = this.createRequest(workerIndex, callback, importers); + this.requests.set(request.id, request); + + this.workers[workerIndex].postMessage({ + id: request.id, + source, + hasImporter: !!importers?.length, + options: { + ...serializableOptions, + // URL is not serializable so to convert to string here and back to URL in the worker. + url: url ? fileURLToPath(url) : undefined, + }, + }); }); } @@ -147,19 +189,11 @@ export class SassWorkerImplementation { this.availableWorkers.push(request.workerIndex); if (response.result) { - // The results are expected to be Node.js `Buffer` objects but will each be transferred as - // a Uint8Array that does not have the expected `toString` behavior of a `Buffer`. - const { css, map, stats } = response.result; - const result: CompileResult = { - // This `Buffer.from` override will use the memory directly and avoid making a copy - css: Buffer.from(css.buffer, css.byteOffset, css.byteLength), - stats, - }; - if (map) { - // This `Buffer.from` override will use the memory directly and avoid making a copy - result.map = Buffer.from(map.buffer, map.byteOffset, map.byteLength); - } - request.callback(undefined, result); + request.callback(undefined, { + ...response.result, + // URL is not serializable so in the worker we convert to string and here back to URL. + loadedUrls: response.result.loadedUrls.map((p) => pathToFileURL(p)), + }); } else { request.callback(response.error); } @@ -167,17 +201,7 @@ export class SassWorkerImplementation { mainImporterPort.on( 'message', - ({ - id, - url, - prev, - fromImport, - }: { - id: number; - url: string; - prev: string; - fromImport: boolean; - }) => { + ({ id, url, options }: { id: number; url: string; options: FileImporterOptions }) => { const request = this.requests.get(id); if (!request?.importers) { mainImporterPort.postMessage(null); @@ -187,7 +211,7 @@ export class SassWorkerImplementation { return; } - this.processImporters(request.importers, url, prev, fromImport) + this.processImporters(request.importers, url, options) .then((result) => { mainImporterPort.postMessage(result); }) @@ -207,44 +231,40 @@ export class SassWorkerImplementation { } private async processImporters( - importers: Iterable, + importers: Iterable, url: string, - prev: string, - fromImport: boolean, - ): Promise { - let result = null; + options: FileImporterOptions, + ): Promise { for (const importer of importers) { - result = await new Promise((resolve) => { - // Importers can be both sync and async - const innerResult = (importer as AsyncImporter).call( - { fromImport } as ImporterThis, - url, - prev, - resolve, - ); - if (innerResult !== undefined) { - resolve(innerResult); - } - }); + if (this.isImporter(importer)) { + // Importer + throw new Error('Only File Importers are supported.'); + } + // File importer (Can be sync or aync). + const result = await importer.findFileUrl(url, options); if (result) { - break; + return fileURLToPath(result); } } - return result; + return null; } private createRequest( workerIndex: number, callback: RenderCallback, - importer: SyncImporter | AsyncImporter | (SyncImporter | AsyncImporter)[] | undefined, + importers: Importers[] | undefined, ): RenderRequest { return { id: this.idCounter++, workerIndex, callback, - importers: !importer || Array.isArray(importer) ? importer : [importer], + importers, }; } + + private isImporter(value: Importers): value is Importer { + return 'canonicalize' in value && 'load' in value; + } } diff --git a/packages/angular_devkit/build_angular/src/sass/worker.ts b/packages/angular_devkit/build_angular/src/sass/worker.ts index bcd978b3258b..65d168e009d0 100644 --- a/packages/angular_devkit/build_angular/src/sass/worker.ts +++ b/packages/angular_devkit/build_angular/src/sass/worker.ts @@ -6,7 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ -import { ImporterResult, LegacyOptions as Options, renderSync } from 'sass'; +import { Exception, StringOptionsWithImporter, compileString } from 'sass'; +import { fileURLToPath, pathToFileURL } from 'url'; import { MessagePort, parentPort, receiveMessageOnPort, workerData } from 'worker_threads'; /** @@ -19,9 +20,13 @@ interface RenderRequestMessage { */ id: number; /** - * The Sass options to provide to the `dart-sass` render function. + * The contents to compile. */ - options: Options<'sync'>; + source: string; + /** + * The Sass options to provide to the `dart-sass` compile function. + */ + options: Omit, 'url'> & { url?: string }; /** * Indicates the request has a custom importer function on the main thread. */ @@ -38,7 +43,11 @@ const { workerImporterPort, importerSignal } = workerData as { importerSignal: Int32Array; }; -parentPort.on('message', ({ id, hasImporter, options }: RenderRequestMessage) => { +parentPort.on('message', ({ id, hasImporter, source, options }: RenderRequestMessage) => { + if (!parentPort) { + throw new Error('"parentPort" is not defined. Sass worker must be executed as a Worker.'); + } + try { if (hasImporter) { // When a custom importer function is present, the importer request must be proxied @@ -46,24 +55,58 @@ parentPort.on('message', ({ id, hasImporter, options }: RenderRequestMessage) => // This process must be synchronous from the perspective of dart-sass. The `Atomics` // functions combined with the shared memory `importSignal` and the Node.js // `receiveMessageOnPort` function are used to ensure synchronous behavior. - options.importer = function (url, prev) { - Atomics.store(importerSignal, 0, 0); - const { fromImport } = this; - workerImporterPort.postMessage({ id, url, prev, fromImport }); - Atomics.wait(importerSignal, 0, 0); + options.importers = [ + { + findFileUrl: (url, options) => { + Atomics.store(importerSignal, 0, 0); + workerImporterPort.postMessage({ id, url, options }); + Atomics.wait(importerSignal, 0, 0); + + const result = receiveMessageOnPort(workerImporterPort)?.message as string | null; - return receiveMessageOnPort(workerImporterPort)?.message as ImporterResult; - }; + return result ? pathToFileURL(result) : null; + }, + }, + ]; } // The synchronous Sass render function can be up to two times faster than the async variant - const result = renderSync(options); + const result = compileString(source, { + ...options, + // URL is not serializable so to convert to string in the parent and back to URL here. + url: options.url ? pathToFileURL(options.url) : undefined, + }); - parentPort?.postMessage({ id, result }); - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } catch (error: any) { + parentPort.postMessage({ + id, + result: { + ...result, + // URL is not serializable so to convert to string here and back to URL in the parent. + loadedUrls: result.loadedUrls.map((p) => fileURLToPath(p)), + }, + }); + } catch (error) { // Needed because V8 will only serialize the message and stack properties of an Error instance. - const { formatted, file, line, column, message, stack } = error; - parentPort?.postMessage({ id, error: { formatted, file, line, column, message, stack } }); + if (error instanceof Exception) { + const { span, message, stack, sassMessage, sassStack } = error; + parentPort.postMessage({ + id, + error: { + span: { + ...span, + url: span.url ? fileURLToPath(span.url) : undefined, + }, + message, + stack, + sassMessage, + sassStack, + }, + }); + } else if (error instanceof Error) { + const { message, stack } = error; + parentPort.postMessage({ id, error: { message, stack } }); + } else { + parentPort.postMessage({ id, error: { message: 'An unknown error has occurred.' } }); + } } }); diff --git a/packages/angular_devkit/build_angular/src/webpack/configs/styles.ts b/packages/angular_devkit/build_angular/src/webpack/configs/styles.ts index f522790b68b6..cb340df9dd5d 100644 --- a/packages/angular_devkit/build_angular/src/webpack/configs/styles.ts +++ b/packages/angular_devkit/build_angular/src/webpack/configs/styles.ts @@ -79,7 +79,7 @@ export function getStylesConfig(wco: WebpackConfigOptions): Configuration { const postcssImports = require('postcss-import'); const postcssPresetEnv: typeof import('postcss-preset-env') = require('postcss-preset-env'); - const { root, buildOptions } = wco; + const { root, projectRoot, buildOptions } = wco; const extraPlugins: Configuration['plugins'] = []; extraPlugins.push(new AnyComponentStyleBudgetChecker(buildOptions.budgets)); @@ -112,6 +112,7 @@ export function getStylesConfig(wco: WebpackConfigOptions): Configuration { } const sassImplementation = new SassWorkerImplementation(); + extraPlugins.push({ apply(compiler) { compiler.hooks.shutdown.tap('sass-worker', () => { @@ -162,7 +163,6 @@ export function getStylesConfig(wco: WebpackConfigOptions): Configuration { : undefined, plugins: [ postcssImports({ - resolve: (url: string) => (url.startsWith('~') ? url.slice(1) : url), load: (filename: string) => { return new Promise((resolve, reject) => { loader.fs.readFile(filename, (err: Error, data: Buffer) => { @@ -270,24 +270,14 @@ export function getStylesConfig(wco: WebpackConfigOptions): Configuration { }, { loader: require.resolve('sass-loader'), - options: { - implementation: sassImplementation, - sourceMap: true, - sassOptions: { - // Prevent use of `fibers` package as it no longer works in newer Node.js versions - fiber: false, - // bootstrap-sass requires a minimum precision of 8 - precision: 8, - includePaths, - // Use expanded as otherwise sass will remove comments that are needed for autoprefixer - // Ex: /* autoprefixer grid: autoplace */ - // See: https://github.com/webpack-contrib/sass-loader/blob/45ad0be17264ceada5f0b4fb87e9357abe85c4ff/src/getSassOptions.js#L68-L70 - outputStyle: 'expanded', - // Silences compiler warnings from 3rd party stylesheets - quietDeps: !buildOptions.verbose, - verbose: buildOptions.verbose, - }, - }, + options: getSassLoaderOptions( + root, + projectRoot, + sassImplementation, + includePaths, + false, + !buildOptions.verbose, + ), }, ], }, @@ -302,25 +292,14 @@ export function getStylesConfig(wco: WebpackConfigOptions): Configuration { }, { loader: require.resolve('sass-loader'), - options: { - implementation: sassImplementation, - sourceMap: true, - sassOptions: { - // Prevent use of `fibers` package as it no longer works in newer Node.js versions - fiber: false, - indentedSyntax: true, - // bootstrap-sass requires a minimum precision of 8 - precision: 8, - includePaths, - // Use expanded as otherwise sass will remove comments that are needed for autoprefixer - // Ex: /* autoprefixer grid: autoplace */ - // See: https://github.com/webpack-contrib/sass-loader/blob/45ad0be17264ceada5f0b4fb87e9357abe85c4ff/src/getSassOptions.js#L68-L70 - outputStyle: 'expanded', - // Silences compiler warnings from 3rd party stylesheets - quietDeps: !buildOptions.verbose, - verbose: buildOptions.verbose, - }, - }, + options: getSassLoaderOptions( + root, + projectRoot, + sassImplementation, + includePaths, + true, + !buildOptions.verbose, + ), }, ], }, @@ -415,3 +394,39 @@ function getTailwindConfigPath({ projectRoot, root }: WebpackConfigOptions): str return undefined; } + +function getSassLoaderOptions( + root: string, + projectRoot: string, + implementation: SassWorkerImplementation, + includePaths: string[], + indentedSyntax: boolean, + verbose: boolean, +): Record { + return { + sourceMap: true, + api: 'modern', + implementation, + // Webpack importer is only implemented in the legacy API. + // See: https://github.com/webpack-contrib/sass-loader/blob/997f3eb41d86dd00d5fa49c395a1aeb41573108c/src/utils.js#L642-L651 + webpackImporter: false, + sassOptions: { + loadPaths: [ + ...includePaths, + // Needed to resolve node packages and retain the same behaviour of with the legacy API as sass-loader resolves + // scss also from the cwd and project root. + // See: https://github.com/webpack-contrib/sass-loader/blob/997f3eb41d86dd00d5fa49c395a1aeb41573108c/src/utils.js#L307 + projectRoot, + path.join(root, 'node_modules'), + ], + // Use expanded as otherwise sass will remove comments that are needed for autoprefixer + // Ex: /* autoprefixer grid: autoplace */ + // See: https://github.com/webpack-contrib/sass-loader/blob/45ad0be17264ceada5f0b4fb87e9357abe85c4ff/src/getSassOptions.js#L68-L70 + style: 'expanded', + // Silences compiler warnings from 3rd party stylesheets + quietDeps: !verbose, + verbose, + syntax: indentedSyntax ? 'indented' : 'scss', + }, + }; +}