Skip to content

Bugfix: Handle TypedArrays and Buffers in deep-merge/clone #1744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ykw9263 wants to merge 13 commits into
base: master
Choose a base branch
from
+236 −5
Open

Bugfix: Handle TypedArrays and Buffers in deep-merge/clone #1744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
6d0ceb8
Clone TypedArray instead of deep merging
ykw9263 Jun 25, 2026
4fdaf60
Add unit tests for new deep merge behaviour
ykw9263 Jun 25, 2026
505b44b
Fix typos in test cases
ykw9263 Jun 25, 2026
3052cee
Fix table fonts to merge shallowly
ykw9263 Jun 25, 2026
f5ccc92
Rewrite table font unit test
ykw9263 Jun 26, 2026
05f624a
Remove unneeded type checks / clone
ykw9263 Jun 27, 2026
fbc34f1
Update CHANGELOG.md
ykw9263 Jun 27, 2026
d724450
Apply suggestions from code review
ykw9263 Jun 28, 2026
731a4c1
Update function name and Lint
ykw9263 Jun 28, 2026
4b84c30
Remove family name in table fonts unit test
ykw9263 Jun 28, 2026
84cb1a1
Bind font src and family in style merge
ykw9263 Jun 28, 2026
283e00b
Merge remote-tracking branch 'upstream/master' into bugfix/table-deep…
ykw9263 Jun 28, 2026
b8ca3a9
Merge font only when font has src defined
ykw9263 Jun 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
- [BREAKING CHANGE] Restrict AcroForm options to documented mappings and explicit escape hatches.
- [BREAKING CHANGE] Stop automatically uppercasing annotation option keys.
- Do not mutate options passed to `doc.annotate()` and its convenience methods (link, note, strike, lineAnnotation, rectAnnotation, ellipseAnnotation, textAnnotation, fileAnnotation)
- Fix Table style merging crashes when passing fonts as buffer (#1743)

### [v0.19.1] - 2026-06-10

Expand Down
7 changes: 6 additions & 1 deletion lib/table/normalize.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,12 @@ export function normalizeCell(cell, rowIndex, colIndex) {
const colStyle = this._colStyle(colIndex);
let rowStyle = this._rowStyle(rowIndex);

const font = deepMerge({}, colStyle.font, rowStyle.font, cell.font);
const font = {
...((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,
};
Comment on lines +97 to +101

@blikblum blikblum Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This breaks the logic of merging

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

@blikblum blikblum Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now i see the size. Forgive me

const customFont = Object.values(font).filter((v) => v != null).length > 0;
const doc = this.document;

Expand Down
22 changes: 20 additions & 2 deletions lib/table/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,17 @@ export function normalizedRowStyle(defaultRowStyle, rowStyleInternal, i) {
rowStyle.borderColor = normalizeSides(rowStyle.borderColor);
rowStyle.align = normalizeAlignment(rowStyle.align);

// extract fonts
const { font: defaultFont, ...restDefaultStyle } = defaultRowStyle || {};
const { font: font, ...restStyle } = rowStyle || {};
const mergedFont = {
...((font?.src && font) || (defaultFont?.src && defaultFont)),
size: font?.size || defaultFont?.size,
};

// Merge defaults
rowStyle = deepMerge(defaultRowStyle, rowStyle);
rowStyle = deepMerge(restDefaultStyle, restStyle);
rowStyle.font = mergedFont;

const document = this.document;
const page = document.page;
Expand Down Expand Up @@ -114,8 +123,17 @@ export function normalizedColumnStyle(defaultColStyle, colStyleInternal, i) {
colStyle.borderColor = normalizeSides(colStyle.borderColor);
colStyle.align = normalizeAlignment(colStyle.align);

// extract fonts
const { font: defaultFont, ...restDefaultStyle } = defaultColStyle || {};
const { font: font, ...restStyle } = colStyle || {};
const mergedFont = {
...((font?.src && font) || (defaultFont?.src && defaultFont)),
size: font?.size || defaultFont?.size,
};
Comment on lines +126 to +132

@blikblum blikblum Jun 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@ykw9263 ykw9263 Jun 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

@blikblum blikblum Jun 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@ykw9263 ykw9263 Jun 29, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@blikblum blikblum Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@blikblum blikblum Jun 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)

@ykw9263 ykw9263 Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

@ykw9263 ykw9263 Jun 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

  1. The most specific src & size will apply
  2. if specificity of family >= specificity of src, family will also apply. Otherwise family is undefined.

@ykw9263 ykw9263 Jun 29, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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?

@hollandjake hollandjake Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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


// Merge defaults
colStyle = deepMerge(defaultColStyle, colStyle);
colStyle = deepMerge(restDefaultStyle, restStyle);
colStyle.font = mergedFont;

if (colStyle.width == null || colStyle.width === '*') {
colStyle.width = '*';
Expand Down
15 changes: 13 additions & 2 deletions lib/table/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,15 @@ function isObject(item) {
return item && typeof item === 'object' && !Array.isArray(item);
}

/**
* buffer check.
* @param item
* @returns {boolean}
*/
function isBinaryData(item) {
return item instanceof Uint8Array || item instanceof ArrayBuffer;
}

/**
* Deep merge two objects.
*
Expand All @@ -345,7 +354,7 @@ export function deepMerge(target, ...sources) {
for (const source of sources) {
if (isObject(source)) {
for (const key in source) {
if (isObject(source[key])) {
if (isObject(source[key]) && !isBinaryData(source[key])) {
if (!(key in target)) target[key] = {};
target[key] = deepMerge(target[key], source[key]);
} else if (source[key] !== undefined) {
Expand All @@ -360,7 +369,9 @@ export function deepMerge(target, ...sources) {

function deepClone(obj) {
let result = obj;
if (obj && typeof obj == 'object') {
if (isBinaryData(obj)) {
result = obj;
} else if (obj && typeof obj == 'object') {
result = Array.isArray(obj) ? [] : {};
for (const key in obj) result[key] = deepClone(obj[key]);
}
Expand Down
196 changes: 196 additions & 0 deletions tests/unit/table.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { vi } from 'vitest';
import PDFDocument from '../../lib/document';
import PDFTable from '../../lib/table';
import { deepMerge } from '../../lib/table/utils';
import fs from 'fs';

describe('table', () => {
test('created', () => {
Expand All @@ -14,6 +16,190 @@ describe('table', () => {
table.row(['A', 'B', 'C']);
expect(table._columnWidths.length).toBe(3);
});

describe('font', () => {
test('column font', () => {
const standardFont = 'Courier';
const fontPath = 'tests/fonts/Roboto-Regular.ttf';
const fontBuffer = fs.readFileSync('tests/fonts/Roboto-Regular.ttf');
const document = new PDFDocument();
const fontSpy = vi.spyOn(document, 'font');

const table = document.table({
columnStyles: [
{ font: { src: standardFont } },
{ font: { src: fontPath } },
{ font: { src: fontBuffer } },
],
});
table.row(['A', 'B', 'C']);
expect(fontSpy).toHaveBeenCalledWith(
standardFont,
expect.toSatisfy(() => true),
);
expect(fontSpy).toHaveBeenCalledWith(
fontPath,
expect.toSatisfy(() => true),
);
expect(fontSpy).toHaveBeenCalledWith(
fontBuffer,
expect.toSatisfy(() => true),
);
});

test('row font', () => {
const standardFont = 'Courier';
const fontPath = 'tests/fonts/Roboto-Regular.ttf';
const fontBuffer = fs.readFileSync('tests/fonts/Roboto-Regular.ttf');
const document = new PDFDocument();
const fontSpy = vi.spyOn(document, 'font');

const table = document.table({
rowStyles: [
{ font: { src: standardFont } },
{ font: { src: fontPath } },
{ font: { src: fontBuffer } },
],
});
table.row(['A']);
table.row(['B']);
table.row(['C']);
expect(fontSpy).toHaveBeenCalledWith(
standardFont,
expect.toSatisfy(() => true),
);
expect(fontSpy).toHaveBeenCalledWith(
fontPath,
expect.toSatisfy(() => true),
);
expect(fontSpy).toHaveBeenCalledWith(
fontBuffer,
expect.toSatisfy(() => true),
);
});

test('cell font', () => {
const standardFont = 'Courier';
const fontPath = 'tests/fonts/Roboto-Regular.ttf';
const fontBuffer = fs.readFileSync('tests/fonts/Roboto-Regular.ttf');
const document = new PDFDocument();
const fontSpy = vi.spyOn(document, 'font');

const table = document.table();
table.row([
{ text: 'A', font: { src: standardFont } },
{ text: 'B', font: { src: fontPath } },
{ text: 'C', font: { src: fontBuffer } },
]);
expect(fontSpy).toHaveBeenCalledWith(
standardFont,
expect.toSatisfy(() => true),
);
expect(fontSpy).toHaveBeenCalledWith(
fontPath,
expect.toSatisfy(() => true),
);
expect(fontSpy).toHaveBeenCalledWith(
fontBuffer,
expect.toSatisfy(() => true),
);
});

test('merge table font', () => {
const fontSrcs = {
colStandardFont: 'Courier',
colFontPath: 'tests/fonts/Roboto-Regular.ttf',
colFontBuffer: fs.readFileSync('tests/fonts/Roboto-Regular.ttf'),
rowStandardFont: 'Courier-Bold',
rowFontPath: 'tests/fonts/Roboto-Medium.ttf',
rowFontBuffer: fs.readFileSync('tests/fonts/Roboto-Medium.ttf'),
cellStandardFont: 'Courier-Oblique',
cellFontPath: 'tests/fonts/Roboto-MediumItalic.ttf',
cellFontBuffer: fs.readFileSync('tests/fonts/Roboto-MediumItalic.ttf'),
};
const fontSrcSet = Object.values(fontSrcs);

/**
* Check whether given spy has been called with specified allowed fonts
* and not other fonts within concerned font set
* @param {*} fontSpy
* @param {import('../../lib/table/utils').Font[]} allowedFonts
*/
function expectFonts(
fontSpy,
allowedFonts = [],
testedFonts = fontSrcSet,
) {
const allowedFontSrc = allowedFonts.map((font) => {
expect(fontSpy).toHaveBeenCalledWith(font.src, font.family);
return font.src;
});
testedFonts.forEach((fontSrc) => {
if (!allowedFontSrc.includes(fontSrc)) {
expect(fontSpy).not.toHaveBeenCalledWith(
fontSrc,
expect.toSatisfy(() => true),
);
}
});
}
const document = new PDFDocument();
const fontSpy = vi.spyOn(document, 'font');

const table = document.table({
columnStyles: [
{ font: { src: fontSrcs.colStandardFont } },
{ font: { src: fontSrcs.colFontPath } },
{ font: { src: fontSrcs.colFontBuffer } },
],
rowStyles: [
{},
{ font: { src: fontSrcs.rowStandardFont } },
{ font: { src: fontSrcs.rowFontPath } },
{ font: { src: fontSrcs.rowFontBuffer } },
{ font: { src: fontSrcs.rowFontBuffer } },
],
});
// fonts in column styles
fontSpy.mockClear();
table.row([{ text: 'A' }, { text: 'B' }, { text: 'C' }]);
expectFonts(fontSpy, [
{ src: fontSrcs.colStandardFont },
{ src: fontSrcs.colFontPath },
{ src: fontSrcs.colFontBuffer },
]);

// fonts in column + row styles
fontSpy.mockClear();
table.row([{ text: 'A' }, { text: 'B' }, { text: 'C' }]);
expectFonts(fontSpy, [{ src: fontSrcs.rowStandardFont }]);
fontSpy.mockClear();
table.row([{ text: 'A' }, { text: 'B' }, { text: 'C' }]);
expectFonts(fontSpy, [{ src: fontSrcs.rowFontPath }]);
fontSpy.mockClear();
table.row([{ text: 'A' }, { text: 'B' }, { text: 'C' }]);
expectFonts(fontSpy, [{ src: fontSrcs.rowFontBuffer }]);

// fonts in column + row + cell style
fontSpy.mockClear();
table.row([
{
text: 'A',
font: { src: fontSrcs.cellStandardFont },
},
{ text: 'B', font: { src: fontSrcs.cellFontPath } },
{
text: 'C',
font: { src: fontSrcs.cellFontBuffer },
},
]);
expectFonts(fontSpy, [
{ src: fontSrcs.cellStandardFont },
{ src: fontSrcs.cellFontPath },
{ src: fontSrcs.cellFontBuffer },
]);
});
});
});

describe('utils', () => {
Expand All @@ -28,6 +214,16 @@ describe('utils', () => {
[1, {}, 1],
[{ a: 'hello' }, { a: {} }, { a: 'hello' }],
[{ a: { b: 'hello' } }, { a: { b: 'world' } }, { a: { b: 'world' } }],
[
{ a: Buffer.from([1, 2, 3]) },
{ b: Buffer.from([4, 5, 6]) },
{ a: Buffer.from([1, 2, 3]), b: Buffer.from([4, 5, 6]) },
],
[
{ a: new Uint8Array([1, 2, 3]) },
{ b: new Uint8Array([4, 5, 6]) },
{ a: new Uint8Array([1, 2, 3]), b: new Uint8Array([4, 5, 6]) },
],
])('%o -> %o', function () {
const opts = Array.from(arguments);
const expected = opts.splice(-1, 1)[0];
Expand Down
Loading