diff --git a/CHANGELOG.md b/CHANGELOG.md index 92ddbe6..c3d2d20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,40 @@ +## 2.5.3 (2026-10-13) + +Fixes PDF uploads in `common-file-uploader`. In an app that never sets +`PdfService.workerSrc`, every PDF, valid or not, was rejected with the +`CannotOpenPDF` error ("That PDF cannot be opened, please upload a different +attachment." in MSP). Affects 2.1.0 through 2.5.2, every release that has +shipped `PdfService`. Image uploads were not affected. + +### Breaking + +- None. No input, output or public API changes, and the `pdfjs-dist` peer + range is unchanged. + +### Added + +- None. + +### Fixed + +- `PdfService` (`common-file-uploader`): with `workerSrc` unset, pdfjs-dist 4.x + has no main-thread fallback of its own. `getDocument()` threw + `No "GlobalWorkerOptions.workerSrc" specified.`, which the uploader reports + as `CannotOpenPDF`. The doc comment said pdfjs would fall back to the main + thread; it did not. The service now imports + `pdfjs-dist/legacy/build/pdf.worker.mjs` and registers it as + `globalThis.pdfjsWorker`, so pdfjs runs the worker on the main thread. The + import keeps its `.catch()`, so `pdfjs-dist` stays an optional peer. pdfjs is + cached only after the worker is registered, so selecting several PDFs at once + or retrying after a failed load works. +- Apps that set `PdfService.workerSrc` are unchanged. + +### Consumer action required + +- None to adopt. Apps on 2.1.0 through 2.5.2 that accept PDFs get the fix by + upgrading to 2.5.3. An app pinned by its lockfile must update the lockfile + (`npm install moh-common-lib-angular@2.5.3`). + ## 2.5.2 (2026-10-12) Fixes the error text a required `common-date` shows for an impossible day. With diff --git a/package-lock.json b/package-lock.json index 9876e3b..466252f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "moh-common-lib-angular", - "version": "2.5.2", + "version": "2.5.3", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "moh-common-lib-angular", - "version": "2.5.2", + "version": "2.5.3", "dependencies": { "@angular/common": "^19.2.20", "@angular/compiler": "^19.2.20", diff --git a/package.json b/package.json index 0301efc..cfddfc0 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "moh-common-lib-angular", - "version": "2.5.2", + "version": "2.5.3", "scripts": { "ng": "ng", "start": "ng serve", diff --git a/projects/common-lib/package.json b/projects/common-lib/package.json index 71be8c0..28a4eee 100644 --- a/projects/common-lib/package.json +++ b/projects/common-lib/package.json @@ -1,6 +1,6 @@ { "name": "moh-common-lib-angular", - "version": "2.5.2", + "version": "2.5.3", "description": "Shared Angular component library for BC Ministry of Health applications.", "keywords": [ "bcgov", diff --git a/src/lib/components/file-uploader/pdf.service.real.spec.ts b/src/lib/components/file-uploader/pdf.service.real.spec.ts new file mode 100644 index 0000000..ce8cdaa --- /dev/null +++ b/src/lib/components/file-uploader/pdf.service.real.spec.ts @@ -0,0 +1,56 @@ +import { deserialize, serialize } from 'v8'; +import { PdfService } from './pdf.service'; + +// Minimal one-page PDF with a correct xref table. +const minimalPdf = (): Uint8Array => { + const objs = [ + '<< /Type /Catalog /Pages 2 0 R >>', + '<< /Type /Pages /Kids [3 0 R] /Count 1 >>', + '<< /Type /Page /Parent 2 0 R /MediaBox [0 0 10 10] >>', + ]; + let out = '%PDF-1.4\n'; + const offsets = objs.map((body, i) => { + const at = out.length; + out += `${i + 1} 0 obj\n${body}\nendobj\n`; + return at; + }); + const xref = out.length; + out += `xref\n0 ${objs.length + 1}\n0000000000 65535 f \n`; + offsets.forEach((o) => (out += `${String(o).padStart(10, '0')} 00000 n \n`)); + out += `trailer\n<< /Size ${objs.length + 1} /Root 1 0 R >>\nstartxref\n${xref}\n%%EOF\n`; + return new TextEncoder().encode(out); +}; + +// Real pdfjs-dist, no mocks. pdfjs decides at module load whether it is in +// Node (pdf.mjs:4967) and if so sets its own workerSrc and loads its own worker +// (pdf.mjs:17315-17317), which hides the browser-only workerSrc error. Jest +// reports a Node-like process, so process.toString is overridden while pdfjs +// first evaluates, forcing the browser path (pdf.mjs:17493-17513). +describe('PdfService with the real pdfjs-dist', () => { + it('opens a PDF in the browser path with workerSrc unset', async () => { + PdfService.workerSrc = undefined; + delete (globalThis as { pdfjsWorker?: unknown }).pdfjsWorker; + const own = Object.getOwnPropertyDescriptor(process, 'toString'); + Object.defineProperty(process, 'toString', { + value: () => '[object browser]', + configurable: true, + }); + // jsdom lacks structuredClone, which pdfjs's main-thread worker port calls. + const g = globalThis as { structuredClone?: unknown }; + const hadClone = g.structuredClone; + g.structuredClone = (v: unknown) => deserialize(serialize(v)); + const service = new PdfService(); + let doc: { numPages: number }; + try { + doc = await service.getDocument(minimalPdf()); + } finally { + g.structuredClone = hadClone; + if (own) { + Object.defineProperty(process, 'toString', own); + } else { + delete (process as any).toString; + } + } + expect(doc.numPages).toBe(1); + }); +}); diff --git a/src/lib/components/file-uploader/pdf.service.spec.ts b/src/lib/components/file-uploader/pdf.service.spec.ts index c5232d4..a3154fb 100644 --- a/src/lib/components/file-uploader/pdf.service.spec.ts +++ b/src/lib/components/file-uploader/pdf.service.spec.ts @@ -8,9 +8,44 @@ const pdfjsMock = { getDocument: jest.fn(), }; -jest.mock('pdfjs-dist/legacy/build/pdf.mjs', () => pdfjsMock, { - virtual: true, -}); +// Hooks called from inside the module factories, so a test can act while load() +// is part-way through its imports. +let onPdfjsImport: (() => void) | undefined; +let onWorkerImport: (() => unknown) | undefined; + +jest.mock( + 'pdfjs-dist/legacy/build/pdf.mjs', + () => { + onPdfjsImport?.(); + return pdfjsMock; + }, + { virtual: true } +); + +const globalWithWorker = () => + globalThis as { pdfjsWorker?: { WorkerMessageHandler?: unknown } }; + +const pdfjsWorkerMock = { WorkerMessageHandler: {} }; + +jest.mock( + 'pdfjs-dist/legacy/build/pdf.worker.mjs', + () => onWorkerImport?.() ?? pdfjsWorkerMock, + { virtual: true } +); + +const noWorkerMessage = 'No "GlobalWorkerOptions.workerSrc" specified.'; + +// Mirrors the check in pdfjs-dist 4.10.38 legacy/build/pdf.mjs (lines 17493-17505): +// getDocument() throws unless a worker handler is on globalThis or workerSrc is set. +const pdfjsLikeGetDocument = () => { + if ( + !globalWithWorker().pdfjsWorker?.WorkerMessageHandler && + !pdfjsMock.GlobalWorkerOptions.workerSrc + ) { + throw new Error(noWorkerMessage); + } + return { promise: Promise.resolve('doc') }; +}; describe('PdfService', () => { let service: PdfService; @@ -19,6 +54,12 @@ describe('PdfService', () => { TestBed.configureTestingModule({}); service = TestBed.inject(PdfService); PdfService.workerSrc = undefined; + onPdfjsImport = undefined; + onWorkerImport = undefined; + // Fresh registry so the module factories (and their hooks) run again in each + // test. + jest.resetModules(); + delete globalWithWorker().pdfjsWorker; pdfjsMock.GlobalWorkerOptions.workerSrc = ''; pdfjsMock.getDocument.mockReset(); pdfjsMock.getDocument.mockReturnValue({ promise: Promise.resolve('doc') }); @@ -50,12 +91,76 @@ describe('PdfService', () => { ); }); - it('should leave pdfjs alone when no workerSrc is configured', async () => { + it('should not register the bundled worker when a workerSrc is configured', async () => { + PdfService.workerSrc = 'https://example.test/pdf.worker.mjs'; + + await service.getDocument(new ArrayBuffer(8)); + + expect(globalWithWorker().pdfjsWorker).toBeUndefined(); + }); + + it('should register the bundled worker for pdfjs when no workerSrc is configured', async () => { pdfjsMock.GlobalWorkerOptions.workerSrc = 'untouched'; await service.getDocument(new ArrayBuffer(8)); expect(pdfjsMock.GlobalWorkerOptions.workerSrc).toBe('untouched'); + expect(globalWithWorker().pdfjsWorker?.WorkerMessageHandler).toBe( + pdfjsWorkerMock.WorkerMessageHandler + ); + }); + + it('should not call pdfjs getDocument before the worker is registered, even for concurrent first calls', async () => { + pdfjsMock.getDocument.mockImplementation(pdfjsLikeGetDocument); + // Holds the worker import open, like a worker chunk still downloading. The + // gate matters: with the second call made inside the factory the race window + // is a couple of microtasks, and an early-cache mutant passes once + // getDocument() gains two awaits. + let releaseWorker!: () => void; + const workerFetched = new Promise((r) => (releaseWorker = r)); + onWorkerImport = () => ({ + __esModule: true, + then: (resolve: (m: unknown) => void) => + workerFetched.then(() => resolve(pdfjsWorkerMock)), + }); + const settle = () => new Promise((r) => setTimeout(r)); + + const first = service.getDocument(new ArrayBuffer(8)); + await settle(); + const second = service.getDocument(new ArrayBuffer(8)); + await settle(); + releaseWorker(); + + await expect(Promise.all([first, second])).resolves.toEqual([ + 'doc', + 'doc', + ]); + }); + + it('should retry the worker import on a later call after it failed', async () => { + onWorkerImport = () => { + onWorkerImport = undefined; + throw new Error('worker chunk failed'); + }; + + await expect(service.getDocument(new ArrayBuffer(8))).rejects.toThrow( + 'uploading a PDF needs pdfjs-dist' + ); + + await service.getDocument(new ArrayBuffer(8)); + expect(globalWithWorker().pdfjsWorker?.WorkerMessageHandler).toBe( + pdfjsWorkerMock.WorkerMessageHandler + ); + }); + + it('should report a missing pdfjs-dist when the pdfjs import fails', async () => { + onPdfjsImport = () => { + throw new Error('Cannot find module'); + }; + + await expect(service.getDocument(new ArrayBuffer(8))).rejects.toThrow( + 'moh-common-lib-angular: uploading a PDF needs pdfjs-dist.' + ); }); // Caching is observable: load() only touches pdfjs behind its `if (!this.pdfjs)` diff --git a/src/lib/components/file-uploader/pdf.service.ts b/src/lib/components/file-uploader/pdf.service.ts index 06aeacd..8669650 100644 --- a/src/lib/components/file-uploader/pdf.service.ts +++ b/src/lib/components/file-uploader/pdf.service.ts @@ -18,25 +18,41 @@ export class PdfService { /** * Where pdfjs should load its worker from. Set this before the first PDF is - * processed. Left unset, pdfjs falls back to running on the main thread, - * which works but blocks while rendering. + * processed. Left unset, the worker is bundled with the app and pdfjs runs it + * on the main thread, which works but blocks while rendering. */ static workerSrc: string | undefined; /** Loads pdfjs on first use, so it is not pulled in unless a PDF is opened. */ private async load(): Promise { if (!this.pdfjs) { - // The .catch() is what keeps this optional. Without it a bundler resolves - // the specifier at build time and fails when pdfjs-dist is absent. - this.pdfjs = await import('pdfjs-dist/legacy/build/pdf.mjs').catch(() => { + // The .catch() on each import is what keeps pdfjs-dist optional. Without + // it a bundler resolves the specifier at build time and fails when + // pdfjs-dist is absent. + const missing = () => { throw new Error( 'moh-common-lib-angular: uploading a PDF needs pdfjs-dist. ' + 'Install it, or restrict common-file-uploader to images.' ); - }); + }; + const pdfjs = await import('pdfjs-dist/legacy/build/pdf.mjs').catch( + missing + ); if (PdfService.workerSrc) { - this.pdfjs.GlobalWorkerOptions.workerSrc = PdfService.workerSrc; + pdfjs.GlobalWorkerOptions.workerSrc = PdfService.workerSrc; + } else { + // With no workerSrc, pdfjs reads globalThis.pdfjsWorker and runs that + // handler on the main thread; with neither, getDocument() throws + // 'No "GlobalWorkerOptions.workerSrc" specified.' + const worker = await import( + // @ts-expect-error pdfjs-dist ships no type declarations for the worker + 'pdfjs-dist/legacy/build/pdf.worker.mjs' + ).catch(missing); + (globalThis as { pdfjsWorker?: unknown }).pdfjsWorker = worker; } + // Cached last, so a concurrent or retried call never sees pdfjs without + // its worker. + this.pdfjs = pdfjs; } return this.pdfjs; }