From 11826f6a38bf1b364eebb4a411eec6181994f280 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bendeg=C3=BAz=20Hajnal?= Date: Wed, 8 Nov 2023 12:27:19 +0100 Subject: [PATCH 1/3] feat(qwik-city): allow customizing SVGO options of image plugin SVGO has a sensible defaults, which are used by qwik, but configurability from developer space is missing, and the default config can cause some unexpected results. For example, objects defined inside can collide with each other, causing unexpected references to be loaded. See reproduction here: https://github.com/hbendev/qwik-svg-render-bug/tree/571a2057914b334a3a3220865868e85a7eb731c3. These modifications allow the 'prefixIds' plugin to be used, and in turn, SVGO prefixes the ids with the svg file's name, making them all unique. --- .../qwik-city-middleware-request-handler/index.md | 2 +- packages/qwik-city/buildtime/vite/api.md | 2 ++ packages/qwik-city/buildtime/vite/image-jsx.ts | 15 +++++++++++++-- .../qwik-city/buildtime/vite/image-jsx.unit.ts | 2 +- packages/qwik-city/buildtime/vite/types.ts | 5 +++++ 5 files changed, 22 insertions(+), 4 deletions(-) diff --git a/packages/docs/src/routes/api/qwik-city-middleware-request-handler/index.md b/packages/docs/src/routes/api/qwik-city-middleware-request-handler/index.md index 4629b930bee..f7638d9a9f9 100644 --- a/packages/docs/src/routes/api/qwik-city-middleware-request-handler/index.md +++ b/packages/docs/src/routes/api/qwik-city-middleware-request-handler/index.md @@ -261,7 +261,7 @@ export interface RequestEventBase | Property | Modifiers | Type | Description | | ----------------- | --------------------- | ------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | | [basePathname](#) | readonly | string | The base pathname of the request, which can be configured at build time. Defaults to /. | -| [cacheControl](#) | readonly | (cacheControl: [CacheControl](#cachecontrol), target?: CacheControlTarget) => void |

Convenience method to set the Cache-Control header. Depending on your CDN, you may want to add another cacheControl with the second argument set to CDN-Cache-Control or any other value (we provide the most common values for auto-complete, but you can use any string you want).

See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control and https://qwik.builder.io/docs/caching/#CDN-Cache-Controls for more information.

| +| [cacheControl](#) | readonly | (cacheControl: [CacheControl](#cachecontrol), target?: CacheControlTarget) => void |

Convenience method to set the Cache-Control header. Depending on your CDN, you may want to add another cacheControl with the second argument set to CDN-Cache-Control or any other value (we provide the most common values for auto-complete, but you can use any string you want).

See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control and https://qwik.builder.io/docs/caching/\#CDN-Cache-Controls for more information.

| | [clientConn](#) | readonly | [ClientConn](#clientconn) | Provides information about the client connection, such as the IP address and the country the request originated from. | | [cookie](#) | readonly | [Cookie](#cookie) |

HTTP request and response cookie. Use the get() method to retrieve a request cookie value. Use the set() method to set a response cookie value.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies

| | [env](#) | readonly | [EnvGetter](#envgetter) | Platform provided environment variables. | diff --git a/packages/qwik-city/buildtime/vite/api.md b/packages/qwik-city/buildtime/vite/api.md index 4a7f1f2b6cf..72543555826 100644 --- a/packages/qwik-city/buildtime/vite/api.md +++ b/packages/qwik-city/buildtime/vite/api.md @@ -4,7 +4,9 @@ ```ts +import type { BuiltinsWithOptionalParams } from 'svgo/plugins/plugins-types'; import { CompileOptions } from '@mdx-js/mdx/lib/compile'; +import type { Config } from 'svgo'; import { ConfigEnv } from 'vite'; import type { PluginOption } from 'vite'; import { UserConfigExport } from 'vite'; diff --git a/packages/qwik-city/buildtime/vite/image-jsx.ts b/packages/qwik-city/buildtime/vite/image-jsx.ts index 5ff3d125ea2..a652296ad0a 100644 --- a/packages/qwik-city/buildtime/vite/image-jsx.ts +++ b/packages/qwik-city/buildtime/vite/image-jsx.ts @@ -91,7 +91,7 @@ export function imagePlugin(userOpts?: QwikCityVitePluginOptions): PluginOption[ }` ); } else if (extension === '.svg') { - const { svgAttributes } = optimizeSvg(code); + const { svgAttributes } = optimizeSvg({ code, pathId }, userOpts); return ` import { _jsxQ } from '@builder.io/qwik'; const PROPS = ${JSON.stringify(svgAttributes)}; @@ -106,15 +106,25 @@ export function imagePlugin(userOpts?: QwikCityVitePluginOptions): PluginOption[ ]; } -export function optimizeSvg(code: string) { +export function optimizeSvg( + { code, pathId }: { code: string; pathId: string }, + userOpts?: QwikCityVitePluginOptions +) { const svgAttributes: Record = {}; + // note: would be great if it warned users if they tried to use qwik-default plugins, so that name collisions are avoided + const userPlugins = userOpts?.imageOptimization?.svgo?.plugins || []; + const data = optimize(code, { + floatPrecision: userOpts?.imageOptimization?.svgo?.floatPrecision, + multipass: userOpts?.imageOptimization?.svgo?.multipass, + path: pathId, plugins: [ { name: 'preset-default', params: { overrides: { removeViewBox: false, + ...userOpts?.imageOptimization?.svgo?.defaultPresetOverrides, }, }, }, @@ -134,6 +144,7 @@ export function optimizeSvg(code: string) { }; }, }, + ...userPlugins, ], }).data; diff --git a/packages/qwik-city/buildtime/vite/image-jsx.unit.ts b/packages/qwik-city/buildtime/vite/image-jsx.unit.ts index d99602da517..1267d2a5d2e 100644 --- a/packages/qwik-city/buildtime/vite/image-jsx.unit.ts +++ b/packages/qwik-city/buildtime/vite/image-jsx.unit.ts @@ -5,7 +5,7 @@ import { optimizeSvg } from './image-jsx'; const svg = ``; test('optimize svg', () => { - const { data, svgAttributes } = optimizeSvg(svg); + const { data, svgAttributes } = optimizeSvg({ code: svg, pathId: '' }); const html = svgAttributes.dangerouslySetInnerHTML; assert.isTrue(data.startsWith('') && data.endsWith('')); diff --git a/packages/qwik-city/buildtime/vite/types.ts b/packages/qwik-city/buildtime/vite/types.ts index d5effff683a..4453a207449 100644 --- a/packages/qwik-city/buildtime/vite/types.ts +++ b/packages/qwik-city/buildtime/vite/types.ts @@ -1,5 +1,7 @@ import type { MdxTransform } from '../markdown/mdx'; import type { BuildContext, BuildEntry, BuildRoute, PluginOptions, MdxPlugins } from '../types'; +import type { Config as SVGOConfig } from 'svgo'; +import type { BuiltinsWithOptionalParams as SVGOBuiltinPluginsWithOptionalParams } from 'svgo/plugins/plugins-types'; /** @public */ export interface ImageOptimizationOptions { @@ -10,6 +12,9 @@ export interface ImageOptimizationOptions { h?: string; [key: string]: string | undefined; }; + svgo?: Pick & { + defaultPresetOverrides?: SVGOBuiltinPluginsWithOptionalParams['preset-default']['overrides']; + }; enabled?: boolean | 'only-production'; } From a1bdaee935f20e4f5a443dcb56bfb86d8f8f5c48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bendeg=C3=BAz=20Hajnal?= Date: Sun, 12 Nov 2023 14:41:04 +0100 Subject: [PATCH 2/3] Add test for the feature --- .../qwik-city/buildtime/vite/image-jsx.ts | 6 +- .../buildtime/vite/image-jsx.unit.ts | 64 ++++++++++++++++++- 2 files changed, 64 insertions(+), 6 deletions(-) diff --git a/packages/qwik-city/buildtime/vite/image-jsx.ts b/packages/qwik-city/buildtime/vite/image-jsx.ts index a652296ad0a..6dc0543c309 100644 --- a/packages/qwik-city/buildtime/vite/image-jsx.ts +++ b/packages/qwik-city/buildtime/vite/image-jsx.ts @@ -91,7 +91,7 @@ export function imagePlugin(userOpts?: QwikCityVitePluginOptions): PluginOption[ }` ); } else if (extension === '.svg') { - const { svgAttributes } = optimizeSvg({ code, pathId }, userOpts); + const { svgAttributes } = optimizeSvg({ code, path: pathId }, userOpts); return ` import { _jsxQ } from '@builder.io/qwik'; const PROPS = ${JSON.stringify(svgAttributes)}; @@ -107,7 +107,7 @@ export function imagePlugin(userOpts?: QwikCityVitePluginOptions): PluginOption[ } export function optimizeSvg( - { code, pathId }: { code: string; pathId: string }, + { code, path }: { code: string; path: string }, userOpts?: QwikCityVitePluginOptions ) { const svgAttributes: Record = {}; @@ -117,7 +117,7 @@ export function optimizeSvg( const data = optimize(code, { floatPrecision: userOpts?.imageOptimization?.svgo?.floatPrecision, multipass: userOpts?.imageOptimization?.svgo?.multipass, - path: pathId, + path: path, plugins: [ { name: 'preset-default', diff --git a/packages/qwik-city/buildtime/vite/image-jsx.unit.ts b/packages/qwik-city/buildtime/vite/image-jsx.unit.ts index 1267d2a5d2e..20daf809e9a 100644 --- a/packages/qwik-city/buildtime/vite/image-jsx.unit.ts +++ b/packages/qwik-city/buildtime/vite/image-jsx.unit.ts @@ -1,14 +1,72 @@ import { assert, test } from 'vitest'; import { optimizeSvg } from './image-jsx'; -// Qwik logo svg -const svg = ``; +const qwikLogoSvg = ``; test('optimize svg', () => { - const { data, svgAttributes } = optimizeSvg({ code: svg, pathId: '' }); + const { data, svgAttributes } = optimizeSvg({ code: qwikLogoSvg, path: '' }); const html = svgAttributes.dangerouslySetInnerHTML; assert.isTrue(data.startsWith('') && data.endsWith('')); assert.isDefined(html); assert.isTrue(html.startsWith('')); }); + +const svgsFilesWithDefsTag = [ + { + content: ` + + + + + + + + SVG +`, + path: '/path/to/svg/svg_1.svg', + }, + { + content: ` + + + + + + + + SVG + `, + path: '/path/to/svg/svg_2.svg', + }, +]; + +test('optimize svgs by path', () => { + const defaultOptimizedSvgs = svgsFilesWithDefsTag.map((file) => + optimizeSvg({ code: file.content, path: file.path }) + ); + + assert.isTrue( + defaultOptimizedSvgs.every((svg) => svg.data.startsWith(' + optimizeSvg( + { code: file.content, path: file.path }, + { + imageOptimization: { + svgo: { + plugins: ['prefixIds'], + }, + }, + } + ) + ); + + // ids should have different names if prefixIds plugin is explicitly added by users + assert.isFalse( + optimizedSvgsWithUserConfig.every((svg) => + svg.data.startsWith(' Date: Sun, 12 Nov 2023 15:07:15 +0100 Subject: [PATCH 3/3] Better assertion for the test --- packages/qwik-city/buildtime/vite/image-jsx.unit.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/qwik-city/buildtime/vite/image-jsx.unit.ts b/packages/qwik-city/buildtime/vite/image-jsx.unit.ts index 20daf809e9a..a303f5b534c 100644 --- a/packages/qwik-city/buildtime/vite/image-jsx.unit.ts +++ b/packages/qwik-city/buildtime/vite/image-jsx.unit.ts @@ -65,7 +65,7 @@ test('optimize svgs by path', () => { // ids should have different names if prefixIds plugin is explicitly added by users assert.isFalse( - optimizedSvgsWithUserConfig.every((svg) => + optimizedSvgsWithUserConfig.some((svg) => svg.data.startsWith('