Skip to content

Commit 83d1d23

Browse files
alan-agius4clydin
authored andcommittedApr 12, 2024·
feat(@angular-devkit/build-angular): enhance Sass rebasing importer for resources URL defined in variables and handling of external paths
This commit introduces enhancements to the Sass rebasing importer, enabling it to resolve resources whose paths are stored in variables or namespaced variables. Also this addresses an issue where URL paths in Sass and SCSS files, flagged as external, were incorrectly rebased, leading to resolution errors. Closes #27445 and closes #27007
1 parent f3024aa commit 83d1d23

File tree

3 files changed

+136
-44
lines changed

3 files changed

+136
-44
lines changed
 

‎packages/angular_devkit/build_angular/src/builders/application/tests/behavior/stylesheet-url-resolution_spec.ts

+115-29
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
*/
88

99
import { buildApplication } from '../../index';
10+
import { OutputHashing } from '../../schema';
1011
import { APPLICATION_BUILDER_INFO, BASE_OPTIONS, describeBuilder } from '../setup';
1112

1213
describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => {
@@ -164,46 +165,131 @@ describeBuilder(buildApplication, APPLICATION_BUILDER_INFO, (harness) => {
164165
);
165166
});
166167

167-
it('should not rebase a URL with a namespaced Sass variable reference', async () => {
168-
await harness.writeFile(
169-
'src/styles.scss',
170-
`
171-
@import "a";
172-
`,
173-
);
174-
await harness.writeFile(
175-
'src/a.scss',
176-
`
177-
@use './b' as named;
178-
.a {
179-
background-image: url(named.$my-var)
180-
}
181-
`,
182-
);
183-
await harness.writeFile(
184-
'src/b.scss',
185-
`
186-
@forward './c.scss' show $my-var;
187-
`,
188-
);
189-
await harness.writeFile(
190-
'src/c.scss',
191-
`
192-
$my-var: "https://example.com/example.png";
193-
`,
194-
);
168+
it('should not rebase a URL with a namespaced Sass variable reference that points to an absolute asset', async () => {
169+
await harness.writeFiles({
170+
'src/styles.scss': `@use 'theme/a';`,
171+
'src/theme/a.scss': `
172+
@use './b' as named;
173+
.a {
174+
background-image: url(named.$my-var)
175+
}
176+
`,
177+
'src/theme/b.scss': `@forward './c.scss' show $my-var;`,
178+
'src/theme/c.scss': `$my-var: "https://example.com/example.png";`,
179+
});
195180

196181
harness.useTarget('build', {
197182
...BASE_OPTIONS,
198183
styles: ['src/styles.scss'],
199184
});
200185

201186
const { result } = await harness.executeOnce();
202-
expect(result?.success).toBe(true);
187+
expect(result?.success).toBeTrue();
203188

204189
harness
205190
.expectFile('dist/browser/styles.css')
206191
.content.toContain('url(https://example.com/example.png)');
207192
});
193+
194+
it('should not rebase a URL with a Sass variable reference that points to an absolute asset', async () => {
195+
await harness.writeFiles({
196+
'src/styles.scss': `@use 'theme/a';`,
197+
'src/theme/a.scss': `
198+
@import './b';
199+
.a {
200+
background-image: url($my-var)
201+
}
202+
`,
203+
'src/theme/b.scss': `$my-var: "https://example.com/example.png";`,
204+
});
205+
206+
harness.useTarget('build', {
207+
...BASE_OPTIONS,
208+
styles: ['src/styles.scss'],
209+
});
210+
211+
const { result } = await harness.executeOnce();
212+
expect(result?.success).toBeTrue();
213+
214+
harness
215+
.expectFile('dist/browser/styles.css')
216+
.content.toContain('url(https://example.com/example.png)');
217+
});
218+
219+
it('should rebase a URL with a namespaced Sass variable referencing a local resource', async () => {
220+
await harness.writeFiles({
221+
'src/styles.scss': `@use 'theme/a';`,
222+
'src/theme/a.scss': `
223+
@use './b' as named;
224+
.a {
225+
background-image: url(named.$my-var)
226+
}
227+
`,
228+
'src/theme/b.scss': `@forward './c.scss' show $my-var;`,
229+
'src/theme/c.scss': `$my-var: "./images/logo.svg";`,
230+
'src/theme/images/logo.svg': `<svg></svg>`,
231+
});
232+
233+
harness.useTarget('build', {
234+
...BASE_OPTIONS,
235+
outputHashing: OutputHashing.None,
236+
styles: ['src/styles.scss'],
237+
});
238+
239+
const { result } = await harness.executeOnce();
240+
expect(result?.success).toBeTrue();
241+
242+
harness.expectFile('dist/browser/styles.css').content.toContain(`url("./media/logo.svg")`);
243+
harness.expectFile('dist/browser/media/logo.svg').toExist();
244+
});
245+
246+
it('should rebase a URL with a Sass variable referencing a local resource', async () => {
247+
await harness.writeFiles({
248+
'src/styles.scss': `@use 'theme/a';`,
249+
'src/theme/a.scss': `
250+
@import './b';
251+
.a {
252+
background-image: url($my-var)
253+
}
254+
`,
255+
'src/theme/b.scss': `$my-var: "./images/logo.svg";`,
256+
'src/theme/images/logo.svg': `<svg></svg>`,
257+
});
258+
259+
harness.useTarget('build', {
260+
...BASE_OPTIONS,
261+
outputHashing: OutputHashing.None,
262+
styles: ['src/styles.scss'],
263+
});
264+
265+
const { result } = await harness.executeOnce();
266+
expect(result?.success).toBeTrue();
267+
268+
harness.expectFile('dist/browser/styles.css').content.toContain(`url("./media/logo.svg")`);
269+
harness.expectFile('dist/browser/media/logo.svg').toExist();
270+
});
271+
272+
it('should not process a URL that has been marked as external', async () => {
273+
await harness.writeFiles({
274+
'src/styles.scss': `@use 'theme/a';`,
275+
'src/theme/a.scss': `
276+
.a {
277+
background-image: url("assets/logo.svg")
278+
}
279+
`,
280+
});
281+
282+
harness.useTarget('build', {
283+
...BASE_OPTIONS,
284+
outputHashing: OutputHashing.None,
285+
externalDependencies: ['assets/*'],
286+
styles: ['src/styles.scss'],
287+
});
288+
289+
const { result } = await harness.executeOnce();
290+
expect(result?.success).toBeTrue();
291+
292+
harness.expectFile('dist/browser/styles.css').content.toContain(`url(assets/logo.svg)`);
293+
});
208294
});
209295
});

‎packages/angular_devkit/build_angular/src/tools/esbuild/stylesheets/css-resource-plugin.ts

+17-8
Original file line numberDiff line numberDiff line change
@@ -32,42 +32,51 @@ export function createCssResourcePlugin(cache?: LoadResultCache): Plugin {
3232
name: 'angular-css-resource',
3333
setup(build: PluginBuild): void {
3434
build.onResolve({ filter: /.*/ }, async (args) => {
35+
const { importer, path, kind, resolveDir, namespace, pluginData = {} } = args;
36+
3537
// Only attempt to resolve url tokens which only exist inside CSS.
3638
// Also, skip this plugin if already attempting to resolve the url-token.
37-
if (args.kind !== 'url-token' || args.pluginData?.[CSS_RESOURCE_RESOLUTION]) {
39+
if (kind !== 'url-token' || pluginData[CSS_RESOURCE_RESOLUTION]) {
3840
return null;
3941
}
4042

43+
let [containingDir, resourceUrl] = path.split('||file:', 2);
44+
if (resourceUrl === undefined) {
45+
// This can happen due to early exit checks in rebasing-importer
46+
// logic such as when the url is an external URL.
47+
resourceUrl = containingDir;
48+
containingDir = '';
49+
}
50+
4151
// If root-relative, absolute or protocol relative url, mark as external to leave the
4252
// path/URL in place.
43-
if (/^((?:\w+:)?\/\/|data:|chrome:|#|\/)/.test(args.path)) {
53+
if (/^((?:\w+:)?\/\/|data:|chrome:|#|\/)/.test(resourceUrl)) {
4454
return {
45-
path: args.path,
55+
path: resourceUrl,
4656
external: true,
4757
};
4858
}
4959

50-
const { importer, kind, resolveDir, namespace, pluginData = {} } = args;
5160
pluginData[CSS_RESOURCE_RESOLUTION] = true;
5261

53-
const result = await build.resolve(args.path, {
62+
const result = await build.resolve(resourceUrl, {
5463
importer,
5564
kind,
5665
namespace,
5766
pluginData,
58-
resolveDir,
67+
resolveDir: join(resolveDir, containingDir),
5968
});
6069

6170
if (result.errors.length) {
6271
const error = result.errors[0];
63-
if (args.path[0] === '~') {
72+
if (resourceUrl[0] === '~') {
6473
error.notes = [
6574
{
6675
location: null,
6776
text: 'You can remove the tilde and use a relative path to reference it, which should remove this error.',
6877
},
6978
];
70-
} else if (args.path[0] === '^') {
79+
} else if (resourceUrl[0] === '^') {
7180
error.notes = [
7281
{
7382
location: null,

‎packages/angular_devkit/build_angular/src/tools/sass/rebasing-importer.ts

+4-7
Original file line numberDiff line numberDiff line change
@@ -82,18 +82,15 @@ abstract class UrlRebasingImporter implements Importer<'sync'> {
8282
continue;
8383
}
8484

85-
// Skip if value is a Sass variable.
86-
// Sass variable usage either starts with a `$` or contains a namespace and a `.$`
87-
if (value[0] === '$' || /^\w+\.\$/.test(value)) {
88-
continue;
89-
}
90-
9185
// Skip if root-relative, absolute or protocol relative url
9286
if (/^((?:\w+:)?\/\/|data:|chrome:|#|\/)/.test(value)) {
9387
continue;
9488
}
9589

96-
const rebasedPath = relative(this.entryDirectory, join(stylesheetDirectory, value));
90+
// Sass variable usage either starts with a `$` or contains a namespace and a `.$`
91+
const valueNormalized = value[0] === '$' || /^\w+\.\$/.test(value) ? `#{${value}}` : value;
92+
const rebasedPath =
93+
relative(this.entryDirectory, stylesheetDirectory) + '||file:' + valueNormalized;
9794

9895
// Normalize path separators and escape characters
9996
// https://developer.mozilla.org/en-US/docs/Web/CSS/url#syntax

0 commit comments

Comments
 (0)
Please sign in to comment.