Skip to content

Conversation

@shuding
Copy link
Member

@shuding shuding commented Oct 30, 2025

TODO: This should be an LRU to reduce memory impact.

Before:

CleanShot 2025-10-30 at 20 48 15@2x

After:

CleanShot 2025-10-30 at 20 48 25@2x
@vercel
Copy link

vercel bot commented Oct 30, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
satori-playground Ready Ready Preview Comment Oct 30, 2025 7:50pm
export default class FontLoader {
defaultFont: opentype.Font
fonts = new Map<string, [opentype.Font, Weight?, FontStyle?][]>()
cachedFontResolver = new Map<number, opentype.Font | undefined>()
Copy link

Choose a reason for hiding this comment

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

The cachedFontResolver cache is now instance-level and shared across all getEngine() calls, but the cache key only includes the character code. This will cause incorrect font resolution when getEngine() is called multiple times with different font parameters.

View Details
📝 Patch Details
diff --git a/src/font.ts b/src/font.ts
index fe56931..0406535 100644
--- a/src/font.ts
+++ b/src/font.ts
@@ -95,7 +95,6 @@ const cachedParsedFont = new WeakMap<
 export default class FontLoader {
   defaultFont: opentype.Font
   fonts = new Map<string, [opentype.Font, Weight?, FontStyle?][]>()
-  cachedFontResolver = new Map<number, opentype.Font | undefined>()
 
   constructor(fontOptions: FontOptions[]) {
     this.addFonts(fontOptions)
@@ -289,6 +288,7 @@ export default class FontLoader {
       }
     }
 
+    const cachedFontResolver = new Map<number, opentype.Font | undefined>()
     const resolveFont = (word: string, fallback = true) => {
       const _fonts = [
         ...fonts,
@@ -305,8 +305,8 @@ export default class FontLoader {
       }
 
       const code = word.charCodeAt(0)
-      if (this.cachedFontResolver.has(code))
-        return this.cachedFontResolver.get(code)
+      if (cachedFontResolver.has(code))
+        return cachedFontResolver.get(code)
 
       const font = _fonts.find((_font, index) => {
         return (
@@ -316,7 +316,7 @@ export default class FontLoader {
       })
 
       if (font) {
-        this.cachedFontResolver.set(code, font)
+        cachedFontResolver.set(code, font)
       }
 
       return font

Analysis

Font resolver cache collision when getEngine() called with different font parameters

What fails: The FontLoader.cachedFontResolver cache in src/font.ts is now instance-level and shared across all getEngine() calls, but the cache key only includes the character code. When getEngine() is called multiple times with different fontFamily/fontWeight/fontStyle parameters, the same character resolves to the wrong font because the cache doesn't account for the resolution context.

How to reproduce:

pnpm test -- font.test.tsx --reporter=verbose
# Test: "should use correct fonts" - renders same text twice with different fontFamily
# First div: default fontFamily (serif)
# Second div: fontFamily='MontserratSubrayada'

Result: Character 'H' (and others) cached from first div with serif font gets reused in second div instead of resolving with Montserrat. Image snapshot differs by 2.48% from expected.

Expected: Each getEngine() call should have its own font resolution cache isolated to that call's font context. Characters should resolve with the appropriate fonts for each specific getEngine() call's parameters.

Root cause: Commit a7fbae0 moved cachedFontResolver from a local variable (created fresh for each getEngine() call, line 292 in previous code) to an instance property (line 98), causing cache contamination across calls with different font contexts. The cache key (character code only) doesn't include the font resolution context (fontFamily, fontWeight, fontStyle, locale).

Fix: Moved cachedFontResolver back to being a local variable inside getEngine() method, restoring isolation between calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants