Skip to content

Commit d09b131

Browse files
committed
fixup! fix(@angular/build): scope Sass package resolution caching for stylesheets in node_modules
1 parent 7d1383d commit d09b131

2 files changed

Lines changed: 82 additions & 7 deletions

File tree

‎packages/angular/build/src/tools/esbuild/stylesheets/sass-language.ts‎

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,28 @@ function parsePackageName(url: string): { packageName: string; readonly pathSegm
9999
};
100100
}
101101

102+
/**
103+
* Returns the scope that qualifies the cached package resolutions of a stylesheet. A stylesheet
104+
* within `node_modules` uses the root of its enclosing package, since every file of a package
105+
* resolves its dependencies against the same `node_modules` directories. All other stylesheets use
106+
* the working directory, allowing component stylesheets to share package resolutions.
107+
*/
108+
export function getPackageScope(
109+
containingPath: string | undefined,
110+
workingDirectory: string | undefined,
111+
): string {
112+
// The directory segments of the stylesheet, excluding the file name
113+
const segments = containingPath?.split(/[\\/]/).slice(0, -1) ?? [];
114+
const index = segments.lastIndexOf('node_modules');
115+
if (index === -1) {
116+
return workingDirectory ?? '';
117+
}
118+
119+
const packageNameLength = segments[index + 1]?.[0] === '@' ? 2 : 1;
120+
121+
return segments.slice(0, index + 1 + packageNameLength).join('/');
122+
}
123+
102124
async function compileString(
103125
data: string,
104126
filePath: string,
@@ -123,7 +145,7 @@ async function compileString(
123145

124146
// Caching follows Sass behavior where a given package url will always resolve to the same value
125147
// regardless of its importer's path, except for importers within `node_modules`, which are
126-
// scoped to their own directory. Relative paths are qualified with the containing URL.
148+
// scoped to their enclosing package. Relative paths are qualified with the containing URL.
127149
// A null value indicates that the cached resolution attempt failed to find a location and
128150
// later stage resolution should be attempted. This avoids potentially expensive repeat
129151
// failing resolution attempts.
@@ -155,12 +177,7 @@ async function compileString(
155177
: undefined;
156178
const resolveDir = containingPath ? dirname(containingPath) : workingDirectory;
157179
const isPackage = isPackageUrl(url);
158-
159-
// Package urls from files within `node_modules` are scoped to the directory of the
160-
// importer to isolate nested dependency versions. All other files share the working
161-
// directory, allowing component stylesheets to share package resolutions.
162-
const isNodeModules = /[\\/]node_modules[\\/]/.test(containingPath ?? '');
163-
const scope = isNodeModules ? (resolveDir ?? '') : (workingDirectory ?? '');
180+
const scope = getPackageScope(containingPath, workingDirectory);
164181
const cacheKey = isPackage
165182
? `${scope}:${url}`
166183
: `${options.containingUrl?.href ?? ''}:${url}`;

‎packages/angular/build/src/tools/esbuild/stylesheets/sass-language_spec.ts‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import { pathToFileURL } from 'node:url';
1515
import { SassCompiler } from '../../sass/sass-service';
1616
import {
1717
SassStylesheetLanguage,
18+
getPackageScope,
1819
isPackageUrl,
1920
resetSassWorkerPoolCaches,
2021
shutdownSassWorkerPool,
@@ -59,6 +60,43 @@ describe('sass-language', () => {
5960
});
6061
});
6162

63+
describe('getPackageScope', () => {
64+
const scope = (containingPath?: string) => getPackageScope(containingPath, '/app');
65+
66+
it('should use the working directory for a stylesheet outside node_modules', () => {
67+
expect(scope('/app/src/app.scss')).toBe('/app');
68+
expect(scope('/app/src/foo-node_modules/a.scss')).toBe('/app');
69+
expect(scope('/app/src/node_modules.scss')).toBe('/app');
70+
expect(scope(undefined)).toBe('/app');
71+
});
72+
73+
it('should use the enclosing package root for a stylesheet within node_modules', () => {
74+
expect(scope('/app/node_modules/pkg/a.scss')).toBe('/app/node_modules/pkg');
75+
expect(scope('/app/node_modules/pkg/sub/a.scss')).toBe('/app/node_modules/pkg');
76+
expect(scope('/app/node_modules/@scope/pkg/sub/a.scss')).toBe('/app/node_modules/@scope/pkg');
77+
expect(scope('/app/node_modules/foo-node_modules/a.scss')).toBe(
78+
'/app/node_modules/foo-node_modules',
79+
);
80+
expect(scope('/app/node_modules/a.scss')).toBe('/app/node_modules');
81+
});
82+
83+
it('should use the innermost package root for a nested dependency', () => {
84+
expect(scope('/app/node_modules/dep/node_modules/pkg/a.scss')).toBe(
85+
'/app/node_modules/dep/node_modules/pkg',
86+
);
87+
expect(scope('/app/node_modules/.pnpm/pkg@1.0.0/node_modules/pkg/a.scss')).toBe(
88+
'/app/node_modules/.pnpm/pkg@1.0.0/node_modules/pkg',
89+
);
90+
});
91+
92+
it('should support Windows path separators', () => {
93+
expect(getPackageScope('C:\\app\\node_modules\\pkg\\sub\\a.scss', 'C:\\app')).toBe(
94+
'C:/app/node_modules/pkg',
95+
);
96+
expect(getPackageScope('C:\\app\\src\\app.scss', 'C:\\app')).toBe('C:\\app');
97+
});
98+
});
99+
62100
describe('package resolution caching', () => {
63101
let temporaryRoot: string;
64102
let projectRoot: string;
@@ -205,6 +243,26 @@ describe('sass-language', () => {
205243
expect(resolveRequests.length).toBe(1);
206244
});
207245

246+
it('should share a package resolution between the stylesheets of different folders of a dependency', async () => {
247+
const dependencyRoot = join(projectRoot, 'node_modules', 'dependency');
248+
const first = await compile(join(dependencyRoot, 'sub1', 'styles.scss'));
249+
const second = await compile(join(dependencyRoot, 'sub2', 'styles.scss'));
250+
251+
expect(first).toContain('content: "dependency";');
252+
expect(second).toContain('content: "dependency";');
253+
expect(resolveRequests.length).toBe(1);
254+
});
255+
256+
it('should share a package resolution between the stylesheets of different folders of a scoped package', async () => {
257+
const packageRoot = join(projectRoot, 'node_modules', '@scope', 'pkg');
258+
const first = await compile(join(packageRoot, 'sub1', 'styles.scss'));
259+
const second = await compile(join(packageRoot, 'sub2', 'styles.scss'));
260+
261+
expect(first).toContain('content: "project";');
262+
expect(second).toContain('content: "project";');
263+
expect(resolveRequests.length).toBe(1);
264+
});
265+
208266
it('should resolve a package url of a non-file containing URL from the working directory', async () => {
209267
// The stylesheets of a build have file URLs, but Sass does not limit a containing URL to them.
210268
spyOn(SassCompiler.prototype, 'compileStringAsync').and.callFake(async (_, options) => {

0 commit comments

Comments
 (0)