Use a hash based on content instead of `guid` #30

stryaponoff · 2024-04-09T09:39:59Z

I was experiencing some problems with my project again and here's how I've solved it.

The problem is vite-svg-2-webfont breaks build idempotence: e.g. if I execute vite build && mv dist _dist && vite build and the compare the _dist and dist directories, it will be completely different - for every single file.

This is because we using hashing mechanism to name assets and chunks:

build: {
  rollupOptions: {
    output: {
      assetFileNames: 'assets/[hash][extname]',
      chunkFileNames: 'assets/[hash].js',
      },
    },
  },

And the vite-svg-2-webfont is using guid() to make unique string for each build. This means that webfont reference inside a CSS will be different between two consequent builds, which means that the CSS file name will be changed because its content is changed, this means that CSS file reference will also be changed, and so on...

I was able to fix this behavior by just replacing the guid() call with some hashing algorithm but I decided to go further and fix another more problem: the build-time warnings.

Every time I was running vite build before, the missing assets warnings was thrown into a console, just a multiple lines like this:

/assets/material-design-icons-round-2fm2M2EJ.woff2 referenced in virtual:mdi-icons-round.css didn't resolve at build time, it will remain unchanged to be resolved at runtime

In this MR I refactored the build mechanisms so it will use normal Rollup build process. Due to its limitations while working with CSS and assets, I wasn't able to avoid some sharp corners, unfortunately, but it does the work anyway.

I will add some comments to clarify decisions in place.

src/fixtures/vite.basic.config.ts

stryaponoff · 2024-04-09T09:44:16Z

src/index.ts

+                    const fileContents = Buffer.from(generatedFonts[type]);
+                    const hash = getBufferHash(fileContents);
+                    const filePath = pathJoin(TMP_DIR, `${processedOptions.fontName}-${hash}.${type}`);
+                    ensureDirExistsAndWriteFile(fileContents, filePath); // write font file to a temporary dir


Here we're writing file to a system temporary dir. So this is workaround for a this.emitFile() mechanism which require us to wait a render-hooks which is called too lately.

Because we're emitting the file explicitly, Rollup will successfully find it and will not produce any warnings and will not be switched into a fallback logical branch.

stryaponoff · 2024-04-09T09:46:20Z

src/index.ts

+
+                    const fileContents = Buffer.from(generatedFonts[type]);
+                    const hash = getBufferHash(fileContents);
+                    const filePath = pathJoin(TMP_DIR, `${processedOptions.fontName}-${hash}.${type}`);


I've replaced the guid() call with hash based on a file contents. Technically speaking, there's no need to add some uniqueness here because the tmp dir is already unique, but it will make easier name-based comparation later in generateBundle() hook (which is required to keep external API working).

stryaponoff · 2024-04-09T09:47:26Z

src/index.ts

@@ -143,6 +163,7 @@ export function viteSvgToWebfont<T extends GeneratedFontTypes = GeneratedFontTyp
        },
        buildEnd() {
            ac.abort();
+            rmDir(TMP_DIR);


Just cleaning up the temporary files

stryaponoff · 2024-04-09T09:49:04Z

src/index.ts

+        generateBundle(_, bundle) {
+            for (const { type, href } of tmpGeneratedWebfonts) {
+                for (const chunk of Object.values(bundle)) {
+                    if (chunk.name && href.endsWith(chunk.name)) {


Unfortunately, I can't find a way to get bundled asset filename by its entry name so here's the workaround where we're comparing the generated asset names with the fonts we generated earlier.

atlowChemi

This is truly awesome! 🚀
I struggled with these warnings for a while before giving up and implementing the current implementation, so really nice to have this fixed! 😄

I've left some comments I would appreciate your response on before I merge this 🙂

src/fixtures/vite.basic.config.ts

src/index.ts

src/utils.ts

stryaponoff · 2024-04-12T09:19:53Z

I've left some comments I would appreciate your response on before I merge this 🙂

Looks good. I'll try to fix this ASAP.

- Remove guid() function - Remove guid() tests - Add getBufferString() tests

- Add no-inline config - Rename "build" test to "build:no-inline" - Add "build" test for testing inline configs

stryaponoff · 2024-04-15T06:48:39Z

Done! Here's what have I done:

removed guid() function and its tests
refactored build test to test the inline configs too
added getBufferHash() test

- Fix "build" test

atlowChemi

Looks good!
Highly appreciated 🙏🏽

src/index.test.ts

src/utils.test.ts

src/utils.ts

Co-authored-by: Chemi Atlow <[email protected]>

atlowChemi

Sorry for the back-and-forth, and thanks for the hard work.
Awesome fix!

Improve building process

85c9d4e

stryaponoff commented Apr 9, 2024

View reviewed changes

src/fixtures/vite.basic.config.ts Outdated Show resolved Hide resolved

stryaponoff commented Apr 9, 2024

View reviewed changes

stryaponoff marked this pull request as ready for review April 9, 2024 09:49

atlowChemi reviewed Apr 12, 2024

View reviewed changes

src/fixtures/vite.basic.config.ts Outdated Show resolved Hide resolved

src/index.ts Show resolved Hide resolved

src/utils.ts Show resolved Hide resolved

src/utils.ts Show resolved Hide resolved

stryaponoff added 2 commits April 15, 2024 12:33

PR fixes

728f1f8

- Remove guid() function - Remove guid() tests - Add getBufferString() tests

PR fixes

2d7261b

- Add no-inline config - Rename "build" test to "build:no-inline" - Add "build" test for testing inline configs

PR fixes

1dd1052

- Fix "build" test

atlowChemi approved these changes Apr 15, 2024

View reviewed changes

src/index.test.ts Outdated Show resolved Hide resolved

src/utils.test.ts Outdated Show resolved Hide resolved

src/utils.ts Outdated Show resolved Hide resolved

stryaponoff and others added 3 commits April 15, 2024 20:41

Apply suggestions from code review

525ba9d

Co-authored-by: Chemi Atlow <[email protected]>

Update src/utils.test.ts

f18448e

Co-authored-by: Chemi Atlow <[email protected]>

Update src/utils.ts

fb4aed2

atlowChemi approved these changes Apr 15, 2024

View reviewed changes

atlowChemi changed the title ~~Improve building process~~ Use a hash based on content instead of guid Apr 15, 2024

atlowChemi merged commit 6722807 into atlowChemi:main Apr 15, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a hash based on content instead of `guid` #30

Use a hash based on content instead of `guid` #30

stryaponoff commented Apr 9, 2024

stryaponoff Apr 9, 2024

stryaponoff Apr 9, 2024

stryaponoff Apr 9, 2024

stryaponoff Apr 9, 2024

atlowChemi left a comment •

edited

Loading

stryaponoff commented Apr 12, 2024

stryaponoff commented Apr 15, 2024 •

edited

Loading

atlowChemi left a comment

atlowChemi left a comment

Use a hash based on content instead of guid #30

Use a hash based on content instead of guid #30

Conversation

stryaponoff commented Apr 9, 2024

stryaponoff Apr 9, 2024

Choose a reason for hiding this comment

stryaponoff Apr 9, 2024

Choose a reason for hiding this comment

stryaponoff Apr 9, 2024

Choose a reason for hiding this comment

stryaponoff Apr 9, 2024

Choose a reason for hiding this comment

atlowChemi left a comment • edited Loading

Choose a reason for hiding this comment

stryaponoff commented Apr 12, 2024

stryaponoff commented Apr 15, 2024 • edited Loading

atlowChemi left a comment

Choose a reason for hiding this comment

atlowChemi left a comment

Choose a reason for hiding this comment

Use a hash based on content instead of `guid` #30

Use a hash based on content instead of `guid` #30

atlowChemi left a comment •

edited

Loading

stryaponoff commented Apr 15, 2024 •

edited

Loading