From 0862a38441105af44e7f9660b0e83066f235722e Mon Sep 17 00:00:00 2001 From: Charles Lyding <19598772+clydin@users.noreply.github.com> Date: Mon, 23 Oct 2023 09:53:43 -0400 Subject: [PATCH] refactor(@angular-devkit/build-angular): remove Sass module resolve workarounds The recent version of the Sass compiler (`dart-sass@1.68.0`) now provides additional information within an importer that allows more accurate resolution of node modules packages without several existing workarounds. Previously, the Sass files needed to be pre-processed to extract all `@import` and `@use` paths so that information regarding the containing Sass file could be used to fully resolve the paths. The Sass compiler now provides this information directly. --- .../esbuild/stylesheets/sass-language.ts | 44 ++++-------- .../src/tools/sass/rebasing-importer.ts | 71 +------------------ .../src/tools/sass/sass-service.ts | 36 +++------- .../build_angular/src/tools/sass/worker.ts | 9 ++- .../src/tools/webpack/configs/styles.ts | 29 +++----- 5 files changed, 40 insertions(+), 149 deletions(-) diff --git a/packages/angular_devkit/build_angular/src/tools/esbuild/stylesheets/sass-language.ts b/packages/angular_devkit/build_angular/src/tools/esbuild/stylesheets/sass-language.ts index ebe5d3f7f0..438ff620aa 100644 --- a/packages/angular_devkit/build_angular/src/tools/esbuild/stylesheets/sass-language.ts +++ b/packages/angular_devkit/build_angular/src/tools/esbuild/stylesheets/sass-language.ts @@ -9,11 +9,8 @@ import type { OnLoadResult, PartialMessage, ResolveResult } from 'esbuild'; import { dirname, join, relative } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; -import type { CompileResult, Exception, Syntax } from 'sass'; -import type { - FileImporterWithRequestContextOptions, - SassWorkerImplementation, -} from '../../sass/sass-service'; +import type { CanonicalizeContext, CompileResult, Exception, Syntax } from 'sass'; +import type { SassWorkerImplementation } from '../../sass/sass-service'; import { StylesheetLanguage, StylesheetPluginOptions } from './stylesheet-plugin-factory'; let sassWorkerPool: SassWorkerImplementation | undefined; @@ -39,31 +36,17 @@ export const SassStylesheetLanguage = Object.freeze({ fileFilter: /\.s[ac]ss$/, process(data, file, format, options, build) { const syntax = format === 'sass' ? 'indented' : 'scss'; - const resolveUrl = async (url: string, options: FileImporterWithRequestContextOptions) => { - let result = await build.resolve(url, { + const resolveUrl = async (url: string, options: CanonicalizeContext) => { + let resolveDir = build.initialOptions.absWorkingDir; + if (options.containingUrl) { + resolveDir = dirname(fileURLToPath(options.containingUrl)); + } + + const result = await build.resolve(url, { kind: 'import-rule', - // Use the provided resolve directory from the custom Sass service if available - resolveDir: options.resolveDir ?? build.initialOptions.absWorkingDir, + resolveDir, }); - // If a resolve directory is provided, no additional speculative resolutions are required - if (options.resolveDir) { - return result; - } - - // Workaround to support Yarn PnP and pnpm without access to the importer file from Sass - if (!result.path && options.previousResolvedModules?.size) { - for (const previous of options.previousResolvedModules) { - result = await build.resolve(url, { - kind: 'import-rule', - resolveDir: previous, - }); - if (result.path) { - break; - } - } - } - return result; }; @@ -103,10 +86,7 @@ async function compileString( filePath: string, syntax: Syntax, options: StylesheetPluginOptions, - resolveUrl: ( - url: string, - options: FileImporterWithRequestContextOptions, - ) => Promise, + resolveUrl: (url: string, options: CanonicalizeContext) => Promise, ): Promise { // Lazily load Sass when a Sass file is found if (sassWorkerPool === undefined) { @@ -139,7 +119,7 @@ async function compileString( quietDeps: true, importers: [ { - findFileUrl: (url, options: FileImporterWithRequestContextOptions) => + findFileUrl: (url, options) => resolutionCache.getOrCreate(url, async () => { const result = await resolveUrl(url, options); if (result.path) { diff --git a/packages/angular_devkit/build_angular/src/tools/sass/rebasing-importer.ts b/packages/angular_devkit/build_angular/src/tools/sass/rebasing-importer.ts index 64c06fad6f..4289caae3d 100644 --- a/packages/angular_devkit/build_angular/src/tools/sass/rebasing-importer.ts +++ b/packages/angular_devkit/build_angular/src/tools/sass/rebasing-importer.ts @@ -12,7 +12,7 @@ import { readFileSync, readdirSync } from 'node:fs'; import { basename, dirname, extname, join, relative } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; import type { CanonicalizeContext, Importer, ImporterResult, Syntax } from 'sass'; -import { findImports, findUrls } from './lexer'; +import { findUrls } from './lexer'; /** * A preprocessed cache entry for the files and directories within a previously searched @@ -23,45 +23,6 @@ export interface DirectoryEntry { directories: Set; } -/** - * A prefix that is added to import and use directive specifiers that should be resolved - * as modules and that will contain added resolve directory information. - * - * This functionality is used to workaround the Sass limitation that it does not provide the - * importer file to custom resolution plugins. - */ -const MODULE_RESOLUTION_PREFIX = '__NG_PACKAGE__'; - -function packModuleSpecifier(specifier: string, resolveDir: string): string { - const packed = - MODULE_RESOLUTION_PREFIX + - ';' + - // Encode the resolve directory to prevent unsupported characters from being present when - // Sass processes the URL. This is important on Windows which can contain drive letters - // and colons which would otherwise be interpreted as a URL scheme. - encodeURIComponent(resolveDir) + - ';' + - // Escape characters instead of encoding to provide more friendly not found error messages. - // Unescaping is automatically handled by Sass. - // https://developer.mozilla.org/en-US/docs/Web/CSS/url#syntax - specifier.replace(/[()\s'"]/g, '\\$&'); - - return packed; -} - -function unpackModuleSpecifier(specifier: string): { specifier: string; resolveDir?: string } { - if (!specifier.startsWith(`${MODULE_RESOLUTION_PREFIX};`)) { - return { specifier }; - } - - const values = specifier.split(';', 3); - - return { - specifier: values[2], - resolveDir: decodeURIComponent(values[1]), - }; -} - /** * A Sass Importer base class that provides the load logic to rebase all `url()` functions * within a stylesheet. The rebasing will ensure that the URLs in the output of the Sass compiler @@ -114,27 +75,6 @@ abstract class UrlRebasingImporter implements Importer<'sync'> { updatedContents.update(start, end, rebasedUrl); } - // Add resolution directory information to module specifiers to facilitate resolution - for (const { start, end, specifier } of findImports(contents)) { - // Currently only provide directory information for known/common packages: - // * `@material/` - // * `@angular/` - // - // Comprehensive pre-resolution support may be added in the future. This is complicated by CSS/Sass not - // requiring a `./` or `../` prefix to signify relative paths. A bare specifier could be either relative - // or a module specifier. To differentiate, a relative resolution would need to be attempted first. - if (!specifier.startsWith('@angular/') && !specifier.startsWith('@material/')) { - continue; - } - - updatedContents ??= new MagicString(contents); - updatedContents.update( - start, - end, - `"${packModuleSpecifier(specifier, stylesheetDirectory)}"`, - ); - } - if (updatedContents) { contents = updatedContents.toString(); if (this.rebaseSourceMaps) { @@ -348,10 +288,7 @@ export class ModuleUrlRebasingImporter extends RelativeUrlRebasingImporter { entryDirectory: string, directoryCache: Map, rebaseSourceMaps: Map | undefined, - private finder: ( - specifier: string, - options: CanonicalizeContext & { resolveDir?: string }, - ) => URL | null, + private finder: (specifier: string, options: CanonicalizeContext) => URL | null, ) { super(entryDirectory, directoryCache, rebaseSourceMaps); } @@ -361,9 +298,7 @@ export class ModuleUrlRebasingImporter extends RelativeUrlRebasingImporter { return super.canonicalize(url, options); } - const { specifier, resolveDir } = unpackModuleSpecifier(url); - - let result = this.finder(specifier, { ...options, resolveDir }); + let result = this.finder(url, options); result &&= super.canonicalize(result.href, options); return result; diff --git a/packages/angular_devkit/build_angular/src/tools/sass/sass-service.ts b/packages/angular_devkit/build_angular/src/tools/sass/sass-service.ts index eb525325aa..c533f37291 100644 --- a/packages/angular_devkit/build_angular/src/tools/sass/sass-service.ts +++ b/packages/angular_devkit/build_angular/src/tools/sass/sass-service.ts @@ -6,10 +6,11 @@ * found in the LICENSE file at https://angular.io/license */ -import { dirname, join } from 'node:path'; +import { join } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { MessageChannel, Worker } from 'node:worker_threads'; import { + CanonicalizeContext, CompileResult, Exception, FileImporter, @@ -31,24 +32,6 @@ const MAX_RENDER_WORKERS = maxWorkers; */ type RenderCallback = (error?: Exception, result?: CompileResult) => void; -type FileImporterOptions = Parameters[1]; - -export interface FileImporterWithRequestContextOptions extends FileImporterOptions { - /** - * This is a custom option and is required as SASS does not provide context from which the file is being resolved. - * This breaks Yarn PNP as transitive deps cannot be resolved from the workspace root. - * - * Workaround until https://github.com/sass/sass/issues/3247 is addressed. - */ - previousResolvedModules?: Set; - - /** - * The base directory to use when resolving the request. - * This value is only set if using the rebasing importers. - */ - resolveDir?: string; -} - /** * An object containing the contextual information for a specific render request. */ @@ -58,7 +41,6 @@ interface RenderRequest { callback: RenderCallback; logger?: Logger; importers?: Importers[]; - previousResolvedModules?: Set; } /** @@ -244,7 +226,7 @@ export class SassWorkerImplementation { mainImporterPort.on( 'message', - ({ id, url, options }: { id: number; url: string; options: FileImporterOptions }) => { + ({ id, url, options }: { id: number; url: string; options: CanonicalizeContext }) => { const request = this.requests.get(id); if (!request?.importers) { mainImporterPort.postMessage(null); @@ -256,14 +238,12 @@ export class SassWorkerImplementation { this.processImporters(request.importers, url, { ...options, - previousResolvedModules: request.previousResolvedModules, + // URL is not serializable so in the worker we convert to string and here back to URL. + containingUrl: options.containingUrl + ? pathToFileURL(options.containingUrl as unknown as string) + : null, }) .then((result) => { - if (result) { - request.previousResolvedModules ??= new Set(); - request.previousResolvedModules.add(dirname(result)); - } - mainImporterPort.postMessage(result); }) .catch((error) => { @@ -284,7 +264,7 @@ export class SassWorkerImplementation { private async processImporters( importers: Iterable, url: string, - options: FileImporterWithRequestContextOptions, + options: CanonicalizeContext, ): Promise { for (const importer of importers) { if (this.isImporter(importer)) { diff --git a/packages/angular_devkit/build_angular/src/tools/sass/worker.ts b/packages/angular_devkit/build_angular/src/tools/sass/worker.ts index a942e374a4..2799b6cc13 100644 --- a/packages/angular_devkit/build_angular/src/tools/sass/worker.ts +++ b/packages/angular_devkit/build_angular/src/tools/sass/worker.ts @@ -93,7 +93,14 @@ parentPort.on('message', (message: RenderRequestMessage) => { const proxyImporter: FileImporter<'sync'> = { findFileUrl: (url, options) => { Atomics.store(importerSignal, 0, 0); - workerImporterPort.postMessage({ id, url, options }); + workerImporterPort.postMessage({ + id, + url, + options: { + ...options, + containingUrl: options.containingUrl ? fileURLToPath(options.containingUrl) : null, + }, + }); Atomics.wait(importerSignal, 0, 0); const result = receiveMessageOnPort(workerImporterPort)?.message as string | null; diff --git a/packages/angular_devkit/build_angular/src/tools/webpack/configs/styles.ts b/packages/angular_devkit/build_angular/src/tools/webpack/configs/styles.ts index 13e1b48ee3..889d8ba75c 100644 --- a/packages/angular_devkit/build_angular/src/tools/webpack/configs/styles.ts +++ b/packages/angular_devkit/build_angular/src/tools/webpack/configs/styles.ts @@ -8,16 +8,13 @@ import MiniCssExtractPlugin from 'mini-css-extract-plugin'; import * as path from 'node:path'; -import { pathToFileURL } from 'node:url'; +import { fileURLToPath, pathToFileURL } from 'node:url'; import type { FileImporter } from 'sass'; import type { Configuration, LoaderContext, RuleSetUseItem } from 'webpack'; import { WebpackConfigOptions } from '../../../utils/build-options'; import { useLegacySass } from '../../../utils/environment-options'; import { findTailwindConfigurationFile } from '../../../utils/tailwind'; -import { - FileImporterWithRequestContextOptions, - SassWorkerImplementation, -} from '../../sass/sass-service'; +import { SassWorkerImplementation } from '../../sass/sass-service'; import { SassLegacyWorkerImplementation } from '../../sass/sass-service-legacy'; import { AnyComponentStyleBudgetChecker, @@ -405,28 +402,20 @@ function getSassResolutionImporter( }); return { - findFileUrl: async ( - url, - { fromImport, previousResolvedModules }: FileImporterWithRequestContextOptions, - ): Promise => { + findFileUrl: async (url, { fromImport, containingUrl }): Promise => { if (url.charAt(0) === '.') { // Let Sass handle relative imports. return null; } + let resolveDir = root; + if (containingUrl) { + resolveDir = path.dirname(fileURLToPath(containingUrl)); + } + const resolve = fromImport ? resolveImport : resolveModule; // Try to resolve from root of workspace - let result = await tryResolve(resolve, root, url); - - // Try to resolve from previously resolved modules. - if (!result && previousResolvedModules) { - for (const path of previousResolvedModules) { - result = await tryResolve(resolve, path, url); - if (result) { - break; - } - } - } + const result = await tryResolve(resolve, resolveDir, url); return result ? pathToFileURL(result) : null; },