feat!: drop fs dependency from browser builds - #1763
Conversation
cd81fca to
b463e24
Compare
diegomura
changed the title
| @@ -1,4 +1,4 @@ | |||
| import fs from 'fs'; | |||
| import iccProfile from './data/sRGB_IEC61966_2_1.icc'; | |||
There was a problem hiding this comment.
How it will behave on node?
It will load at library import?
We should try to keep the lazy loading approach
There was a problem hiding this comment.
Both node and browser behaves the same way. icc data is loaded as Uint8Array, converted in build time using binary rollup plugin.
We should try to keep the lazy loading approach
I think this is nice but how without fs?
There was a problem hiding this comment.
Decoding can be made lazy, but not loading. Feels like a very tiny optimization, but added it here
There was a problem hiding this comment.
I will take a look on how to keep the lazy loading behavior.
| }, | ||
| "default": "./js/pdfkit.browser.mjs" | ||
| }, | ||
| "./virtual-fs": { |
There was a problem hiding this comment.
Lets keep virtual fs for now. Is not bundled / used by default and does not hurt at all.
You can remove the usage in webpack
Having a vfs is the only way to register a globally accessible resource in browser.
So my idea is using one in browser #fs and exporting as vfs. This can be done later
There was a problem hiding this comment.
Dont mind restoring virtual fs but after this change what purpose would it serve if runtime doesn't actually use fs in browser?
There was a problem hiding this comment.
I think if we want for this library to be truly isomorphic we should stop relying on node deps at all, that includes virtual-fs workarounds
There was a problem hiding this comment.
I think better. You can keep removal. I have another idea to provide a global file registration
e490f2d to
53ba4b7
Compare
Browser bundles no longer import `fs`. File-path arguments to `registerFont`, `image` and `file` now resolve through a `#fs` subpath import that maps to Node's fs on Node and to a shim that throws a clear error elsewhere. The PDF/A sRGB ICC profile was read from disk via `__dirname`, which never worked outside Node. It is now inlined at build time by a small rollup/vite plugin that turns `.icc` imports into a `Uint8Array`. Removes the virtual file system (`pdfkit/virtual-fs`), which was already broken since the library uses fs APIs it does not implement, along with the now unnecessary `brfs` transform and the runtime data copy step.
53ba4b7 to
ae8fc25
Compare
2 participants
What kind of change does this PR introduce?
Browser builds no longer depend on
fs.Passing a file path to
registerFont,imageorfileonly works on Node now. Everywhere else those paths throw an error telling you to pass aUint8Array,ArrayBufferor data URL instead. The PDF/A ICC profile used to be read from disk with__dirname, so it never worked in a browser anyway. It is now inlined at build time.This is part of modernizing the library. Bundlers no longer polyfill Node builtins by default, and we shouldn't expect users to configure polyfills to get a PDF out of the browser.
It also drops
pdfkit/virtual-fs, which was already broken: the library calls fs APIs the virtual fs never implemented, so it fails as soon as you hit one of them.This is the last thing blocking react-pdf from using pdfkit directly instead of its fork. That should bring roughly double the downloads to pdfkit.