Skip to content

Commit

Permalink
fix(svgo/r config): update handling of prefixIds plugin to prefix svg…
Browse files Browse the repository at this point in the history 
… ID's with a string hash of their filename (#3342)

* fix(svgo/r config): update handling of prefixIds plugin to prefix svg ID's with a string hash of their filename in order to provide usable ID prefixes for SVG's in dev/prod webpack configs and vite-plugin-svgr in mc-scripts

* fix(svgo/r config): remove unused 'svgoPrefixIdsCount' variable from prod webpack config and vite-plugin-svgr

* fix(svgo/r config): remove string-hash dep in favor of nodejs builtin crypto library, use shake256 hash in order to limit the output length of the hash
  • Loading branch information
@ByronDWall
ByronDWall authored Dec 13, 2023
1 parent 3d4d502 commit 5314f16
Show file tree
Hide file tree
Showing 5 changed files with 293 additions and 86 deletions.
7 changes: 7 additions & 0 deletions .changeset/mighty-impalas-provide.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@commercetools-frontend/mc-scripts': patch
---

Avoid implementing 2 separate ID's for ID's that are declared and consumed in the same SVG file in webpack's SVGR loader and Vite SVGR plugin.

More information about the approach can be found [here](https://github.com/svg/svgo/issues/913#issuecomment-369373572).
36 changes: 28 additions & 8 deletions packages/mc-scripts/src/config/create-webpack-config-for-development.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import crypto from 'crypto';
import path from 'path';
import ReactRefreshWebpackPlugin from '@pmmmwh/react-refresh-webpack-plugin';
import HtmlWebpackPlugin from 'html-webpack-plugin';
import MomentLocalesPlugin from 'moment-locales-webpack-plugin';
import type { XastElement, PluginInfo } from 'svgo';
import webpack, { type Configuration } from 'webpack';
import WebpackBar from 'webpackbar';
import type {
Expand All @@ -16,8 +18,6 @@ import momentLocalesToKeep from /* preval */ './moment-locales';
import paths from './paths';
import vendorsToTranspile from './vendors-to-transpile';

let svgoPrefixIdsCount = 0;

const defaultToggleFlags: TWebpackConfigToggleFlagsForDevelopment = {
generateIndexHtml: true,
disableCoreJs: false,
Expand Down Expand Up @@ -253,18 +253,38 @@ function createWebpackConfigForDevelopment(
},
},
// Avoid collisions with ids in other SVGs,
// which was causing incorrect gradient directions
// https://github.com/svg/svgo/issues/1746#issuecomment-1803600573
//
// Previously, this was a problem where unique ids
// could not be generated since svgo@3
// which was causing incorrect masking, gradient directions, etc
// this is an ongoing issue with both SVGR and SVGO,
// https://github.com/svg/svgo/issues/913#issuecomment-369373572
// see SVGR issues:
// https://github.com/gregberge/svgr/issues/322
// https://github.com/gregberge/svgr/issues/210
// see SVGO issues:
// https://github.com/svg/svgo/issues/674
// https://github.com/svg/svgo/issues/1746
//
// Initially, a naive counter was implemented based on this github comment:
// https://github.com/svg/svgo/issues/1746#issuecomment-1803600573
// But while that implementation insured id’s that are unique,
// it did not work in cases where the id is both declared and referenced in the same file,
// because the refernce gets a separate unique ID (a different number from the counter).
//
// The current implementation is based on this github comment:
// https://github.com/svg/svgo/issues/913#issuecomment-369373572
// Generates a hash of the filepath of the svg file, resulting in a prefix which is:
// - Short,
// - With characters valid for IDs,
// - The same within a file,
// - And different in different files.
{
name: 'prefixIds',
params: {
delim: '',
prefix: () => svgoPrefixIdsCount++,
prefix: (_: XastElement, info: PluginInfo) =>
`svg${crypto
.createHash('shake256', { outputLength: 6 })
.update(info.path || '')
.digest('hex')}`,
},
},
],
Expand Down
36 changes: 28 additions & 8 deletions packages/mc-scripts/src/config/create-webpack-config-for-production.ts
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
// @ts-ignore
import crypto from 'crypto';
import CssMinimizerPlugin from 'css-minimizer-webpack-plugin';
import HtmlWebpackPlugin from 'html-webpack-plugin';
import MiniCssExtractPlugin from 'mini-css-extract-plugin';
import MomentLocalesPlugin from 'moment-locales-webpack-plugin';
import type { XastElement, PluginInfo } from 'svgo';
import TerserPlugin from 'terser-webpack-plugin';
import webpack, {
type WebpackPluginInstance,
Expand All @@ -21,8 +23,6 @@ import momentLocalesToKeep from /* preval */ './moment-locales';
import paths from './paths';
import vendorsToTranspile from './vendors-to-transpile';

let svgoPrefixIdsCount = 0;

const defaultToggleFlags: TWebpackConfigToggleFlagsForProduction = {
// Allow to disable CSS extraction in case it's not necessary (e.g. for Storybook)
enableExtractCss: true,
Expand Down Expand Up @@ -273,18 +273,38 @@ function createWebpackConfigForProduction(
},
},
// Avoid collisions with ids in other SVGs,
// which was causing incorrect gradient directions
// https://github.com/svg/svgo/issues/1746#issuecomment-1803600573
//
// Previously, this was a problem where unique ids
// could not be generated since svgo@3
// which was causing incorrect masking, gradient directions, etc
// this is an ongoing issue with both SVGR and SVGO,
// https://github.com/svg/svgo/issues/913#issuecomment-369373572
// see SVGR issues:
// https://github.com/gregberge/svgr/issues/322
// https://github.com/gregberge/svgr/issues/210
// see SVGO issues:
// https://github.com/svg/svgo/issues/674
// https://github.com/svg/svgo/issues/1746
//
// Initially, a naive counter was implemented based on this github comment:
// https://github.com/svg/svgo/issues/1746#issuecomment-1803600573
// But while that implementation insured id’s that are unique,
// it did not work in cases where the id is both declared and referenced in the same file,
// because the refernce gets a separate unique ID (a different number from the counter).
//
// The current implementation is based on this github comment:
// https://github.com/svg/svgo/issues/913#issuecomment-369373572
// Generates a hash of the filepath of the svg file, resulting in a prefix which is:
// - Short,
// - With characters valid for IDs,
// - The same within a file,
// - And different in different files.
{
name: 'prefixIds',
params: {
delim: '',
prefix: () => svgoPrefixIdsCount++,
prefix: (_: XastElement, info: PluginInfo) =>
`svg${crypto
.createHash('shake256', { outputLength: 6 })
.update(info.path || '')
.digest('hex')}`,
},
},
],
Expand Down
36 changes: 28 additions & 8 deletions packages/mc-scripts/src/vite-plugins/vite-plugin-svgr.ts
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
/**
* COPIED FROM https://github.com/pd4d10/vite-plugin-svgr
*/
import crypto from 'crypto';
import fs from 'fs';
import { createFilter } from '@rollup/pluginutils';
import type { XastElement, PluginInfo } from 'svgo';
import { transformWithEsbuild, type Plugin } from 'vite';

let svgoPrefixIdsCount = 0;

function vitePluginSvgr(): Plugin {
const filter = createFilter('**/*.react.svg');
return {
Expand All @@ -32,18 +32,38 @@ function vitePluginSvgr(): Plugin {
},
},
// Avoid collisions with ids in other SVGs,
// which was causing incorrect gradient directions
// https://github.com/svg/svgo/issues/1746#issuecomment-1803600573
//
// Previously, this was a problem where unique ids
// could not be generated since svgo@3
// which was causing incorrect masking, gradient directions, etc
// this is an ongoing issue with both SVGR and SVGO,
// https://github.com/svg/svgo/issues/913#issuecomment-369373572
// see SVGR issues:
// https://github.com/gregberge/svgr/issues/322
// https://github.com/gregberge/svgr/issues/210
// see SVGO issues:
// https://github.com/svg/svgo/issues/674
// https://github.com/svg/svgo/issues/1746
//
// Initially, a naive counter was implemented based on this github comment:
// https://github.com/svg/svgo/issues/1746#issuecomment-1803600573
// But while that implementation insured id’s that are unique,
// it did not work in cases where the id is both declared and referenced in the same file,
// because the refernce gets a separate unique ID (a different number from the counter).
//
// The current implementation is based on this github comment:
// https://github.com/svg/svgo/issues/913#issuecomment-369373572
// Generates a hash of the filepath of the svg file, resulting in a prefix which is:
// - Short,
// - With characters valid for IDs,
// - The same within a file,
// - And different in different files.
{
name: 'prefixIds',
params: {
delim: '',
prefix: () => svgoPrefixIdsCount++ as unknown as string,
prefix: (_: XastElement, info: PluginInfo) =>
`svg${crypto
.createHash('shake256', { outputLength: 6 })
.update(info.path || '')
.digest('hex')}`,
},
},
],
Expand Down
Loading

2 comments on commit 5314f16

@github-actions
Copy link
Contributor

Choose a reason for hiding this comment

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

Deploy preview for application-kit-custom-views ready!

✅ Preview
https://application-kit-custom-views-cbmsz7v62-commercetools.vercel.app

Built with commit 5314f16.
This pull request is being automatically deployed with vercel-action

@github-actions
Copy link
Contributor

Choose a reason for hiding this comment

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

Deploy preview for merchant-center-application-kit ready!

✅ Preview
https://merchant-center-application-ipifujimi-commercetools.vercel.app

Built with commit 5314f16.
This pull request is being automatically deployed with vercel-action

Please sign in to comment.