Bookmark: every bookmark is emitted with w:id="1" (per-instance id generator), so multiple bookmarks collide

[alexvcasillas]

Summary

When a document contains more than one Bookmark, every w:bookmarkStart/w:bookmarkEnd is written with the same w:id="1". The numeric bookmark ids are meant to be unique per document, so multiple bookmarks collide, which makes start/end pairing ambiguous and produces an invalid document (Word cannot reliably resolve PageReference / internal hyperlinks to the intended target).

Reproduction

import { Document, Packer, Paragraph, TextRun, Bookmark } from "docx";

const doc = new Document({
  sections: [{
    children: [
      new Paragraph({ children: [new Bookmark({ id: "AnchorA", children: [new TextRun("A")] })] }),
      new Paragraph({ children: [new Bookmark({ id: "AnchorB", children: [new TextRun("B")] })] }),
    ],
  }],
});

const buf = await Packer.toBuffer(doc);
// unzip word/document.xml

Resulting word/document.xml (abridged):

<w:bookmarkStart w:name="AnchorA" w:id="1"/> ... <w:bookmarkEnd w:id="1"/>
<w:bookmarkStart w:name="AnchorB" w:id="1"/> ... <w:bookmarkEnd w:id="1"/>

Both bookmarks receive w:id="1".

Root cause

In src/file/paragraph/links/bookmark.ts, the id generator is created per Bookmark instance:

private readonly bookmarkUniqueNumericId = bookmarkUniqueNumericIdGen();
// ...
const linkId = this.bookmarkUniqueNumericId();

bookmarkUniqueNumericIdGen() returns a fresh counter starting at 1, and each instance calls its own counter exactly once, so every bookmark gets 1. (Container types such as Numbering share one generator across all their children, which is why their ids increment correctly; Bookmark has no shared container, so each instance is isolated.)

Expected

Each bookmark in a document should receive a distinct numeric w:id, e.g. 1, 2, 3, with the matching bookmarkStart/bookmarkEnd sharing that id.

Suggested fix

Use a single module-scoped generator shared by all Bookmark instances (uniqueness is only required within a document, and a monotonic module-level counter satisfies that), for example:

const bookmarkNumericId = bookmarkUniqueNumericIdGen();

export class Bookmark {
  public constructor(options: IBookmarkOptions) {
    const linkId = bookmarkNumericId();
    this.start = new BookmarkStart(options.id, linkId);
    this.children = options.children;
    this.end = new BookmarkEnd(linkId);
  }
}

Version

Reproduced on docx@9.7.1 and on current master.

[alexvcasillas]

One correction to my own suggestion above, now that #3502 is open.

That PR does not take the module-scoped approach I proposed here. It allocates ids per document instead, through a registry on File that BookmarkStart and BookmarkEnd read during serialization.

A module-level counter does satisfy validity, since ids only need to be unique within a document, but it carries costs I had not thought through when I filed this:

• Anything stubbing bookmarkUniqueNumericIdGen in a test stops intercepting, because module scope is evaluated at import time. That is silent, with no type error, and it broke one of the existing tests in this repo.
• Output stops being reproducible. The second document generated in a process continues from where the first stopped, so the same input produces different bytes.
• The counter never resets and has no bound.
• With two copies of docx loaded, each has its own counter, so the guarantee is per module instance rather than per document.

Per document avoids all of those, and the "Expected" section above still describes what it produces: distinct ids, with each bookmarkStart and bookmarkEnd sharing one.

Leaving the original text in place rather than editing it, so the discussion still reads in order.