Bugfix: Handle TypedArrays and Buffers in deep-merge/clone by ykw9263 · Pull Request #1744 · foliojs/pdfkit

ykw9263 · 2026-06-25T16:40:19Z

What kind of change does this PR introduce?

This fixes #1743 where providing font buffers crashes table generation.

In lib\table\utils.js, deepMerge and deepClone() copy properties key by key including Buffer and Uint8Array without inheriting the class. This causes the merged font buffer failing to be recognised by PDFFontFactory which relies on instanceof to determine the type of font sources.

Also fixes #1746 where table style merging falsely assign font families.

This PR changes:

deepMerge() and deepClone() now clone TypedArrays and Buffers using .slice() and Buffer.from(buffer) respectively.
Table style merging is modified to copy fonts by reference.

Additional unit tests (fonts in tables, deep-merging buffers) are added for the changes.

Checklist:

Unit Tests
Documentation
Update CHANGELOG.md
Ready to be merged

TODO:

blikblum

While the font handling is not overhauled this seems a good workaround

- remove unnecessary isObject check - rename binary data check function Co-authored-by: Luiz Américo <camara_luiz@yahoo.com.br>

- font family name is not allowed for ttf fonts with no Variations

blikblum · 2026-06-28T09:15:10Z

+  // extract fonts
+  const { font: defaultFont, ...restDefaultStyle } = defaultColStyle || {};
+  const { font: font, ...restStyle } = colStyle || {};
+  const mergedFont = {
+    ...defaultFont,
+    ...font,
+  };


Given that deepMerge was modified to handle binary data, why not use it?

Just found a bug where merging fonts with font collections would cause problem

doc.table({ rowStyles: [{ font: { src: SOME_TTC_FONT, family: FONT_FAMILY }, }] }) .row([ { text: 'Hello World', font: { src: SOME_TTF_FONT } } ]); // throws `Variations require a font with the fvar, gvar and glyf, or CFF2 tables`

I think font src should be binded with its family and not be merged with other family names across styles

This is another bug, independent of table, just using font, please open another issue

The bug is caused by style merging in tables propagating font families to other font options. Other than tables the same error seems only be thrown on user errors when they put unsupported families in font options.

Manually assigning font properties and avoiding deepMerge on table fonts fixes this together.

But the changes ignores font: {size}

This is a rabbit hole

@hollandjake how about we keep with the following font cell | row | col def:

font: string |buffer
fontSize: number

We kill most of the complexity and AFAIK still pretty flexible

This is internal doc. Never publicized

In the final shape of this PR it introduces complexity not only in code, but in usage reasoning / documentation (must explain that font.family will not merge like other properties - is ignored without src).

In fact it breaks a valid use case:

defaultStyle: {font.src: 'fontCollection'}

cell1.font.family: 'family1'
cell2.font.family: 'family2'

IMO we should either treat font as an atomic setting or we embrace fully the current merging approach ( changing the type or not)

In fact it breaks a valid use case:

defaultStyle: {font.src: 'fontCollection'} cell1.font.family: 'family1' cell2.font.family: 'family2'

This definitely sounds complex to handle inline. May actually need a dedicated utility function to merge fonts properly without making a mess

/** * Merge font definitions. Front takes precedence * @param {Font[]} fonts * @returns {Font} merged font */ export function mergeFonts(...fonts) { const mergedFont = {}; for (const font of fonts) { if (!mergedFont.size) { mergedFont.size = font?.size; } // ignore font src / family once src is found if (!mergedFont.src){ // take the first defined family if (!mergedFont.family){ mergedFont.family = font?.family; } if (font?.src) { mergedFont.src = font.src; } } // return when font src & size are both defined if (mergedFont.src && mergedFont.size){ return mergedFont; } } return mergedFont; } // const mergedFont = fontMerge(cell.font, rowStyle.font, colStyle.font)

edited for cleaner branches

IMO we should either treat font as an atomic setting or we embrace fully the current merging approach ( changing the type or not)

Agree that the font merging behaviour should be defined especially for partial options. If we choose the full merging approach while accounting the case in #1744 (comment), the cascading logic would need to work like

The most specific src & size will apply

if specificity of family >= specificity of src, family will also apply. Otherwise family is undefined.

BTW seems that defaultStyle.font is never merged into the final font in the original implementation.

pdfkit/lib/table/normalize.js

Line 96 in 0036303

const font = deepMerge({}, colStyle.font, rowStyle.font, cell.font);

Is this intentional?

Yes, thats because the defaults get populated during the normalisation stuff

blikblum · 2026-06-28T09:15:39Z

There is a minor changelog conflict

…-merged-typedarray

blikblum · 2026-06-29T01:31:24Z

+    ...((cell.font?.src && cell.font) ||
+      (rowStyle.font?.src && rowStyle.font) ||
+      (colStyle.font?.src && colStyle.font)),
+    size: cell.font?.size || rowStyle.font?.size || colStyle.font?.size,
+  };


This breaks the logic of merging

If you have a font: {size: 16} in row will be ignored with this last change

Now i see the size. Forgive me

blikblum · 2026-06-29T01:37:23Z

+  // extract fonts
+  const { font: defaultFont, ...restDefaultStyle } = defaultColStyle || {};
+  const { font: font, ...restStyle } = colStyle || {};
+  const mergedFont = {
+    ...defaultFont,
+    ...font,
+  };


But the changes ignores font: {size}

This is a rabbit hole

@hollandjake how about we keep with the following font cell | row | col def:

font: string |buffer
fontSize: number

We kill most of the complexity and AFAIK still pretty flexible

ykw9263 added 3 commits June 26, 2026 00:05

Clone TypedArray instead of deep merging

6d0ceb8

Add unit tests for new deep merge behaviour

4fdaf60

Fix typos in test cases

505b44b

ykw9263 marked this pull request as draft June 25, 2026 17:33

ykw9263 and others added 2 commits June 26, 2026 02:26

Fix table fonts to merge shallowly

3052cee

Rewrite table font unit test

f5ccc92

blikblum reviewed Jun 26, 2026

View reviewed changes

Comment thread lib/table/utils.js Outdated

Comment thread lib/table/utils.js Outdated

ykw9263 and others added 2 commits June 28, 2026 02:51

Remove unneeded type checks / clone

05f624a

Update CHANGELOG.md

fbc34f1

ykw9263 marked this pull request as ready for review June 27, 2026 19:25

blikblum requested changes Jun 27, 2026

View reviewed changes

Comment thread lib/table/utils.js Outdated

Comment thread lib/table/utils.js Outdated

ykw9263 and others added 3 commits June 28, 2026 13:49

Apply suggestions from code review

d724450

- remove unnecessary isObject check - rename binary data check function Co-authored-by: Luiz Américo <camara_luiz@yahoo.com.br>

Update function name and Lint

731a4c1

Remove family name in table fonts unit test

4b84c30

- font family name is not allowed for ttf fonts with no Variations

blikblum reviewed Jun 28, 2026

View reviewed changes

ykw9263 added 3 commits June 28, 2026 17:34

Bind font src and family in style merge

84cb1a1

Merge remote-tracking branch 'upstream/master' into bugfix/table-deep…

283e00b

…-merged-typedarray

Merge font only when font has src defined

b8ca3a9

ykw9263 mentioned this pull request Jun 29, 2026

Table falsely propagate font families when providing fonts in column/row style #1746

Open

blikblum reviewed Jun 29, 2026

View reviewed changes

Uh oh!

Conversation

ykw9263 commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What kind of change does this PR introduce?

This PR changes:

TODO:

Uh oh!

Uh oh!

Uh oh!

blikblum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykw9263 Jun 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykw9263 Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ykw9263 Jun 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blikblum commented Jun 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ykw9263 commented Jun 25, 2026 •

edited

Loading

ykw9263 Jun 28, 2026 •

edited

Loading

ykw9263 Jun 29, 2026 •

edited

Loading

ykw9263 Jun 29, 2026 •

edited

Loading