Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(rollup-plugin-html): glob patterns exclusion for external assets #2671

5 changes: 5 additions & 0 deletions .changeset/nervous-bugs-rhyme.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@web/rollup-plugin-html': minor
---

glob patterns exclusion for external assets
2 changes: 2 additions & 0 deletions docs/docs/building/rollup-plugin-html.md
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,8 @@ export interface RollupPluginHTMLOptions {
transformHtml?: TransformHtmlFunction | TransformHtmlFunction[];
/** Whether to extract and bundle assets referenced in HTML. Defaults to true. */
extractAssets?: boolean;
/** Whether to ignore assets referenced in HTML and CSS with glob patterns. */
externalAssets?: string | string[];
/** Define a full absolute url to your site (e.g. https://domain.com) */
absoluteBaseUrl?: string;
/** Whether to set full absolute urls for ['meta[property=og:image]', 'link[rel=canonical]', 'meta[property=og:url]'] or not. Requires a absoluteBaseUrl to be set. Default to true. */
Expand Down
4 changes: 3 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion packages/rollup-plugin-html/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,12 @@
"glob": "^10.0.0",
"html-minifier-terser": "^7.1.0",
"lightningcss": "^1.24.0",
"parse5": "^6.0.1"
"parse5": "^6.0.1",
"picomatch": "^2.2.2"
},
"devDependencies": {
"@types/html-minifier-terser": "^7.0.0",
"@types/picomatch": "^2.2.1",
"rollup": "^4.4.0"
}
}
2 changes: 2 additions & 0 deletions packages/rollup-plugin-html/src/RollupPluginHTMLOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ export interface RollupPluginHTMLOptions {
transformHtml?: TransformHtmlFunction | TransformHtmlFunction[];
/** Whether to extract and bundle assets referenced in HTML. Defaults to true. */
extractAssets?: boolean;
/** Whether to ignore assets referenced in HTML and CSS with glob patterns. */
externalAssets?: string | string[];
/** Define a full absolute url to your site (e.g. https://domain.com) */
absoluteBaseUrl?: string;
/** Whether to set full absolute urls for ['meta[property=og:image]', 'link[rel=canonical]', 'meta[property=og:url]'] or not. Requires a absoluteBaseUrl to be set. Default to true. */
Expand Down
9 changes: 9 additions & 0 deletions packages/rollup-plugin-html/src/assets/utils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Document, Element } from 'parse5';
import path from 'path';
import picomatch from 'picomatch';
import { findElements, getTagName, getAttribute } from '@web/parse5-utils';
import { createError } from '../utils.js';
import { serialize } from 'v8';
Expand Down Expand Up @@ -143,3 +144,11 @@ export function getSourcePaths(node: Element) {
export function findAssets(document: Document) {
return findElements(document, isAsset);
}

// picomatch follows glob spec and requires "./" to be removed for the matcher to work
// it is safe, because with or without it resolves to the same file
// read more: https://github.com/micromatch/picomatch/issues/77
const removeLeadingSlash = (str: string) => (str.startsWith('./') ? str.slice(2) : str);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we just path.normalize it?

Copy link
Member

@bashmish bashmish Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to keep modifications to the original paths used in source files to a minimum
path.normalize would solve it too, but might make other unexpected modifications which might influence the matcher behavior
not sure we should use that to be honest

export function createAssetPicomatchMatcher(glob?: string | string[]) {
return picomatch(glob || [], { format: removeLeadingSlash });
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was pretty unexpected, but the proposed workaround from the picomatch maintainer seems to be good for us
micromatch/picomatch#77

I added different variations in the test, with and without ./, to be sure it works

Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,28 @@ import {
getSourcePaths,
isHashedAsset,
resolveAssetFilePath,
createAssetPicomatchMatcher,
} from '../../assets/utils.js';

export interface ExtractAssetsParams {
document: Document;
htmlFilePath: string;
htmlDir: string;
rootDir: string;
externalAssets?: string | string[];
absolutePathPrefix?: string;
}

export function extractAssets(params: ExtractAssetsParams): InputAsset[] {
const assetNodes = findAssets(params.document);
const allAssets: InputAsset[] = [];
const isExternal = createAssetPicomatchMatcher(params.externalAssets);

for (const node of assetNodes) {
const sourcePaths = getSourcePaths(node);
for (const sourcePath of sourcePaths) {
if (isExternal(sourcePath)) continue;

const filePath = resolveAssetFilePath(
sourcePath,
params.htmlDir,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,12 @@ export interface ExtractParams {
htmlFilePath: string;
rootDir: string;
extractAssets: boolean;
externalAssets?: string | string[];
absolutePathPrefix?: string;
}

export function extractModulesAndAssets(params: ExtractParams) {
const { html, htmlFilePath, rootDir, absolutePathPrefix } = params;
const { html, htmlFilePath, rootDir, externalAssets, absolutePathPrefix } = params;
const htmlDir = path.dirname(htmlFilePath);
const document = parse(html);

Expand All @@ -24,7 +25,14 @@ export function extractModulesAndAssets(params: ExtractParams) {
absolutePathPrefix,
});
const assets = params.extractAssets
? extractAssets({ document, htmlDir, htmlFilePath, rootDir, absolutePathPrefix })
? extractAssets({
document,
htmlDir,
htmlFilePath,
rootDir,
externalAssets,
absolutePathPrefix,
})
: [];

// turn mutated AST back to a string
Expand Down
8 changes: 7 additions & 1 deletion packages/rollup-plugin-html/src/input/getInputData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,20 @@ export interface CreateInputDataParams {
rootDir: string;
filePath?: string;
extractAssets: boolean;
externalAssets?: string | string[];
absolutePathPrefix?: string;
}

function createInputData(params: CreateInputDataParams): InputData {
const { name, html, rootDir, filePath, extractAssets, absolutePathPrefix } = params;
const { name, html, rootDir, filePath, extractAssets, externalAssets, absolutePathPrefix } =
params;
const htmlFilePath = filePath ? filePath : path.resolve(rootDir, name);
const result = extractModulesAndAssets({
html,
htmlFilePath,
rootDir,
extractAssets,
externalAssets,
absolutePathPrefix,
});

Expand All @@ -63,6 +66,7 @@ export function getInputData(
rootDir = process.cwd(),
flattenOutput,
extractAssets = true,
externalAssets,
absolutePathPrefix,
exclude: ignore,
} = pluginOptions;
Expand All @@ -77,6 +81,7 @@ export function getInputData(
html: input.html,
rootDir,
extractAssets,
externalAssets,
absolutePathPrefix,
});
result.push(data);
Expand All @@ -97,6 +102,7 @@ export function getInputData(
rootDir,
filePath,
extractAssets,
externalAssets,
absolutePathPrefix,
});
result.push(data);
Expand Down
4 changes: 3 additions & 1 deletion packages/rollup-plugin-html/src/output/emitAssets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { transform } from 'lightningcss';
import fs from 'fs';

import { InputAsset, InputData } from '../input/InputData';
import { createAssetPicomatchMatcher } from '../assets/utils.js';
import { RollupPluginHTMLOptions, TransformAssetFunction } from '../RollupPluginHTMLOptions';

export interface EmittedAssets {
Expand Down Expand Up @@ -81,6 +82,7 @@ export async function emitAssets(

let ref: string;
let basename = path.basename(asset.filePath);
const isExternal = createAssetPicomatchMatcher(options.externalAssets);
const emittedExternalAssets = new Map();
if (asset.hashed) {
if (basename.endsWith('.css') && options.bundleAssetsFromCss) {
Expand All @@ -95,7 +97,7 @@ export async function emitAssets(
// https://www.w3.org/TR/html4/types.html#:~:text=ID%20and%20NAME%20tokens%20must,tokens%20defined%20by%20other%20attributes.
const [filePath, idRef] = url.url.split('#');

if (shouldHandleAsset(filePath)) {
if (shouldHandleAsset(filePath) && !isExternal(filePath)) {
// Read the asset file, get the asset from the source location on the FS using asset.filePath
const assetLocation = path.resolve(path.dirname(asset.filePath), filePath);
const assetContent = fs.readFileSync(assetLocation);
Expand Down
1 change: 1 addition & 0 deletions packages/rollup-plugin-html/src/output/getOutputHTML.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ export async function getOutputHTML(params: GetOutputHTMLParams) {
outputDir,
rootDir,
emittedAssets,
externalAssets: pluginOptions.externalAssets,
absolutePathPrefix,
publicPath: pluginOptions.publicPath,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
getSourcePaths,
isHashedAsset,
resolveAssetFilePath,
createAssetPicomatchMatcher,
} from '../assets/utils.js';
import { InputData } from '../input/InputData.js';
import { createError } from '../utils.js';
Expand All @@ -20,6 +21,7 @@ export interface InjectUpdatedAssetPathsArgs {
outputDir: string;
rootDir: string;
emittedAssets: EmittedAssets;
externalAssets?: string | string[];
publicPath?: string;
absolutePathPrefix?: string;
}
Expand All @@ -42,14 +44,18 @@ export function injectedUpdatedAssetPaths(args: InjectUpdatedAssetPathsArgs) {
outputDir,
rootDir,
emittedAssets,
externalAssets,
publicPath = './',
absolutePathPrefix,
} = args;
const assetNodes = findAssets(document);
const isExternal = createAssetPicomatchMatcher(externalAssets);

for (const node of assetNodes) {
const sourcePaths = getSourcePaths(node);
for (const sourcePath of sourcePaths) {
if (isExternal(sourcePath)) continue;

const htmlFilePath = input.filePath ? input.filePath : path.join(rootDir, input.name);
const htmlDir = path.dirname(htmlFilePath);
const filePath = resolveAssetFilePath(sourcePath, htmlDir, rootDir, absolutePathPrefix);
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#a1 {
background-image: url('image-a.png');
}

#a2 {
background-image: url('image-a.svg');
}

#d1 {
background-image: url('./image-d.png');
}

#d2 {
background-image: url('./image-d.svg');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some deduplication of images was happening, I think because we try to identify images with same hash and merge them into one, so it's a feature of this plugin
but it made testing the exclusion cumbersome, so I added a new image-d.svg/png with the content equal to the file name to get a different hash than for image-a and see in the output all expected exclusions

the deduplication continues to work correctly, it's just that test would be hard to read if I mix so much different functionality in one place

}
91 changes: 91 additions & 0 deletions packages/rollup-plugin-html/test/rollup-plugin-html.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1216,6 +1216,97 @@ describe('rollup-plugin-html', () => {

#h {
background-image: url("assets/star-H06WHrYy.avif");
}`.trim(),
);
});

it('allows to exclude external assets usign a glob pattern', async () => {
const config = {
plugins: [
rollupPluginHTML({
input: {
html: `<html>
<head>
<link rel="apple-touch-icon" sizes="180x180" href="./image-a.png" />
<link rel="icon" type="image/png" sizes="32x32" href="image-d.png" />
<link rel="manifest" href="./webmanifest.json" />
<link rel="mask-icon" href="./image-a.svg" color="#3f93ce" />
<link rel="mask-icon" href="image-d.svg" color="#3f93ce" />
<link rel="stylesheet" href="./styles-with-referenced-assets.css" />
<link rel="stylesheet" href="./foo/x.css" />
<link rel="stylesheet" href="foo/bar/y.css" />
</head>
<body>
<img src="./image-d.png" />
<div>
<img src="./image-d.svg" />
</div>
</body>
</html>`,
},
bundleAssetsFromCss: true,
externalAssets: ['**/foo/**/*', '*.svg'],
rootDir: path.join(__dirname, 'fixtures', 'assets'),
}),
],
};

const bundle = await rollup(config);
const { output } = await bundle.generate(outputConfig);

expect(output.length).to.equal(8);

const expectedAssets = [
'assets/image-a.png',
'assets/image-d.png',
'styles-with-referenced-assets.css',
'image-a.png',
'image-d.png',
'webmanifest.json',
];

for (const name of expectedAssets) {
const asset = getAsset(output, name);
expect(asset).to.exist;
expect(asset.source).to.exist;
}

const outputHtml = getAsset(output, 'index.html').source;
expect(outputHtml).to.include(
'<link rel="apple-touch-icon" sizes="180x180" href="assets/image-a.png">',
);
expect(outputHtml).to.include(
'<link rel="icon" type="image/png" sizes="32x32" href="assets/image-d.png">',
);
expect(outputHtml).to.include('<link rel="manifest" href="assets/webmanifest.json">');
expect(outputHtml).to.include('<link rel="mask-icon" href="./image-a.svg" color="#3f93ce">');
expect(outputHtml).to.include('<link rel="mask-icon" href="image-d.svg" color="#3f93ce">');
expect(outputHtml).to.include(
'<link rel="stylesheet" href="assets/styles-with-referenced-assets-NuwIw8gN.css">',
);
expect(outputHtml).to.include('<link rel="stylesheet" href="./foo/x.css">');
expect(outputHtml).to.include('<link rel="stylesheet" href="foo/bar/y.css">');
expect(outputHtml).to.include('<img src="assets/assets/image-d-y8_AQMDl.png">');
expect(outputHtml).to.include('<img src="./image-d.svg">');

const rewrittenCss = getAsset(output, 'styles-with-referenced-assets.css')
.source.toString()
.trim();
expect(rewrittenCss).to.equal(
`#a1 {
background-image: url("assets/image-a-Mr5Lb2jQ.png");
}

#a2 {
background-image: url("image-a.svg");
}

#d1 {
background-image: url("assets/image-d-y8_AQMDl.png");
}

#d2 {
background-image: url("./image-d.svg");
}`.trim(),
);
});
Expand Down
Loading