feat: compatibility with modern web bundlers and browsers #1891
Conversation
carlobeltrame
changed the title
|
Thanks for working on this @carlobeltrame ! Having full support of bundlers is certainly tricky. I have doubts though regarding bundling all those libs inside react-pdf. Wouldn't that have as a consequence duplicated dependencies being bundled in projects? Ex. react-pdf is bundled with I'm not that familiarized with Vite, but there isn't a way to polyfill these objects as it is with webpack5? |
|
Thanks for your concern! I will try to explain why I think bundling the dependencies is the better alternative. Sorry if this is a little long, but I think it's worth to consider all the intricacies of this matter. Vite does have configuration options to alias certain packages to others. But if you want to go that route, it will be necessary to create and maintain either plugins/shims or documentation for all popular current and future web bundlers, of which there are now many (Webpack 4, Webpack 5, Rollup, ESBuild, Vite, Parcel, Snowpack, ...) and there will be more in the future. The times of everyone using Webpack are over - sadly or not, depending on who you ask. While these bundlers all have different ways to configure workarounds such as aliases and polyfills, they do agree on the ES6 module standard. So the end goal for every package in the javascript ecosystem should be to follow that standard. If a package does that, it will work with any of the above bundlers. Bundling these dependencies with react-pdf does prevent reconciling them if a project happens to require the same package in some other place. But consider that this is just a temporary measure: Ideally, an isomorphic package such as react-pdf shouldn't use node builtins at all. So one solution would be to switch react-pdf also on the server side to all isomorphic dependencies, i.e. stop using A good example is the Let's look at the packages which I had to bundle in this PR in a little more detail:
Please also consider that these dependencies are only bundled in the new browser-ready versions of the @react-pdf packages. The node versions should be completely unaffected. |
|
@Belkacem If you want to help out by testing this PR, you can follow the instructions in https://github.com/diegomura/react-pdf/blob/master/.github/CONTRIBUTING.md, they are well written. I followed them myself for working on this PR, and it was quite easy to do. |
|
Hi guys. I've been slowly working on updating my old libraries. When I started working on them (over 10 years ago), there wasn't even a way to represent binary data in JS, so things have come a long way since then 😄. I released restructure v3 and fontkit v2 recently, which drop all node-specific dependencies. Here’s a demo working in the browser with no additional build step. The plan would be to get there with pdfkit as well. It'll take a bit of work to drop Node streams, and other node dependencies like |
|
That sounds absolutely great. Thanks so much for the huge amount of work you did already! I could definitely imagine helping out with replacing node dependencies in pdfkit, but I am still relatively new to the whole PDF ecosystem, so I'll probably start out helping in small pieces. Returning react-pdf to using the original foliojs packages would be great in my opinion. But I don't know why @diegomura chose to hard fork them in the first place. In fontkit for example, I see almost no react-pdf-specific changes, except for #1839 which actually caused another regression for non-webpack browser users. |
|
Hey guys! Sorry for the late response. Been very busy and also sick this past week. Thanks so much @carlobeltrame for such a clear and detailed explanation! This is a field in which I don't really have much experience. I now agree this is the right direction, specially considering @devongovett efforts on making the foliojs packages really isomorphic. I'll test with webpack4 and webpack5. What else you think it's blocking this to get merged? Would love to rely on you to lead this effort 😄 Hey @devongovett ! Nice to talk to you again. Thanks for your new work on the restructure and textkit repos! I would love to get back to the mainline repo. Haven't yet take a look, but is there anywhere I can read about how to migrate to textkit v2? @carlobeltrame how this bump would affect this PR? I'd love to get back to the foliojs packages and join both communities efforts 😄 This is completely my fault. I've been a foliojs maintainer for awhile, but some time ago decided to fork because was simpler to me not having to think about how changes in those packages affected other users of the libs. Maintaining the react-pdf org demands me more time that I have already, so thinking of maintaining two orgs was more than I could take. Also, considering react-pdf is quite big already (mostly due to Yoga), was a good strategy to trim some pieces of the foliojs projects I didn't need and make it smaller. But I now see that leads to repetitive work and sometimes more effort on my side trying to keep up with the changes. I'll make removing the textkit fork a prio |
|
Apart from testing (especially on node.js), CI is currently failing during the size check. I am trying to find out which commit exactly broke it, and find a fix. |
|
Something I haven't confirmed yet it's broken but it might be: in some places we import some textkit files as |
|
@diegomura I haven't touched textkit to be clear - only fontkit. As for migration, you can see the changes here - there weren't many. Upgrade is going to happen in PDFKit here: foliojs/pdfkit#1367. Would be great if you want to test that with react-pdf. Let me know if there are other changes you need back ported from your forks to any of the foliojs repos. |
You mean like here? Yeah, I replaced those imports, by exporting anything that's needed externally from textkit. This is something that might need backporting to foliojs textkit I guess. Not sure what you mean by "broken", but basic text layouting definitely works. I haven't tested svg yet, will do that as well then. |
This is necessary because we had to add some browser-compatible alternatives for node-only dependencies and builtins. See diegomura#1891 for more details.
|
Yes meant fontkit. Typo 😄 I'll work this weekend on trying to get on the fontkit mainline repo. That also includes |
This is necessary because we had to add some browser-compatible alternatives for node-only dependencies and builtins. See diegomura#1891 for more details.
diegomura
force-pushed
the
browser-compatibility
branch
from
2752345
to
bf9a8e4
Compare
packages/png-js/rollup.config.js
Outdated
| const babelConfig = ({ browser }) => ({ | ||
| babelrc: false, | ||
| babelHelpers: 'runtime', | ||
| exclude: 'node_modules/**', | ||
| presets: [ | ||
| [ | ||
| '@babel/preset-env', | ||
| { | ||
| loose: true, | ||
| modules: false, | ||
| targets: { node: '12', browsers: 'last 2 versions' }, | ||
| }, | ||
| }), | ||
| ), | ||
| external: ['fs'], | ||
| ], | ||
| ], | ||
| plugins: [['@babel/plugin-transform-runtime', { version: '^7.16.4' }]], |
There was a problem hiding this comment.
Can we use babelrc: true as config so we don't duplicate config here?
There was a problem hiding this comment.
Done, I went ahead and did this for all packages.
|
@carlobeltrame rebased with upstream changes. Hope you don't mind. Would like to prioritize this! However, testing this branch, building Also, what makes the new |
|
just encountered the same issue. i also tried using the shim but sadly could not get it to work for me. |
|
There was a new usage of |
This is necessary because we had to add some browser-compatible alternatives for node-only dependencies and builtins. See diegomura#1891 for more details.
diegomura
force-pushed
the
browser-compatibility
branch
from
bf9a8e4
to
c8becd1
Compare
|
@carlobeltrame I was just able to run builds. I noticed that |
awesome. i appreciate it. btw does anyone get react-pdf to work with storybook. i am having issue after issue and cant get it to work |
I can have a look at what takes up this much space.
As soon as pdfkit is isomorphic and we switch to that, all these rollup configs can of course be deleted along with the @react-pdf/xyz forks of the packages. |
|
I analyzed the package size using rollup-plugin-sizes. Here is what I found: Before this PR, just app code (~380kB)With this PR, before any manual externalizing (~960kB)After this PR, when externalizing `pako` (~780kB)By externalizing the transitive dependency |
|
From my perspective, this PR is ready to merge now. I also tested a basic SVG. I haven't gotten it to run in a web worker, but I can have a look at that later. @diegomura let me know in case you see anything else you'd like me to change before merging, or if you find anything that does not work in a Node.js environment (which I did not test, because I only develop for the browser). |
|
Thanks so much @carlobeltrame ! You're great. Couple of follow up questions if you don't mind. Not trying to block it (sorry, I would like to merge this asap as well), but trying to still understand some things I think we need externalize Also, what are |
|
@terragady See the issues above. We're waiting for a release. |
|
@Uzaaft yes I know and asking what is the current status of that if it is planned in near future? |
|
And as you can see above, we haven't gotten a response. |
|
well ok you are not really helpful so not sure why you bother commenting. |
|
Unfortunately I don't know any trick which you could use for a productive app, since this is a lerna monorepo which npm and yarn don't understand natively. To test locally whether master works in your project you can follow the instructions in .github/CONTRIBUTING.md and use |
|
For everyone that's waiting for a release, here is a PR that will include these changes as a release that you guys can track |
Thank you @carlobeltrame, this piece is done. I still am facing an issue while viewing in a responsive frame on Chrome. No text is appearing in this frame. Any Reason for this type of behaviour? |
Sorry, but I have no idea what you are talking about, and it definitely has nothing to do with this PR here, since this hasn't even been released. This PR is not a general-purpose support channel for react-pdf. Please open a new issue including a reproduction project or REPL. |
@terragady you can try to use @react-pdf-jepek/renderer instead. I have built the packages and published them to the npm while waiting for #1912 to be done. |
|
@jepek yeah I have used it but there was some error about the _a somewhere and after long fight of upgrading React I just gave up on renderer. I just switched to jspdf for now |
|
@terragady Ah yea, I am aware of it, something went wrong on postbuild script in yoga (subpackage of react-pdf). I will fix that soon so people who want to use react-pdf/[email protected] will be able to use @react-pdf-jepek/renderer BTW I am experimenting and probably will be switching to pdfmake |
|
yes I have considered it as well, just use jspdf because it was easy to change as they have htnl2pdf plugin which seemed to be easy (but it is not :) ). Pdfmake is nice but it does not come with Viewer for example, just "open" and "save", where "open" just works with 3 browsers. I am starting to think to generate PDF on server side, might be easier and better. I am already doing this with shipping labels. |
|
@terragady for displaying the PDF I am using the react-pdf - works fine. client vs server - there are always some tradeoffs - whatever works better for you :) BTW It is so confusing (the names) using react-pdf and @react-pdf/renderer ;) |
|
@jepek
|
Compatibility was fixed in diegomura/react-pdf#1891
|
Hi, I believe this should fix problem with create-react-app v5 using webpack 5. But unfortunately when creating fresh cra application with instruction from quick-start https://react-pdf.org/ I see following errors: here is reproduction repo: https://github.com/Gratify-Ocelot/cra-pdf-test |
|
Thanks for reporting! I see that in #1908, @diegomura added base64-to-uint8array as a new dependency to the font package. This happened more or less in parallel to this PR. The base64-to-uint8array actually doesn't use buffer in a browser context, so the code will still work. But since webpack 5 does not do tree shaking by default, it still tries to compile the node.js code as well, which fails. You can see in the source code that the author tried to work around this problem by splitting the The node.js-specific code using Buffer in that package is actually completely unnecessary nowadays, since Uint8Array is also available on node.js since node 10. So, given that the base64-to-uint8array package has never been updated since its initial commit 5 years ago, I think we could just replace the single use of that package with the single line of code line which is relevant for us. |
|
Cool, should we expect this change in the next version? Seems like a good opportunity to remove some unnecessary dependency |
|
I probably don't have time for it before the second half of the month. If it's urgent for you, you should be able to create the fix yourself, based on the information above. |
|
Sure, no worries, take your time. Is there any possibility to subscribe to some issue or pull request to be notified when it's done? |
|
thanks @thekevinbrown, @carlobeltrame can you create draft issue or pull-request for this change so I can subscribe for notifications? |
|
Sorry, no time. Feel free to open a new bug issue yourself with the info from your first comment. I'll make sure to reference that if I'm the first person to fix it |
Its working for me on local, But when I build app. It throws this error. Please need help here React v18 |
As discussed in #1317, react-pdf currently cannot be used out of the box with modern ES bundlers such as Vite or Rollup (which react-pdf even uses itself for building the various packages). The reason was that react-pdf contains code which references node.js builtin modules such as
Buffer,stream,zlibetc., which are not natively available in browsers.This PR creates new browser-specific builds for all the @react-pdf packages that need it. This way, I was able to leave the node.js-specific builds almost untouched. After this PR, react-pdf can be used in an app without having to add any bundler config (such as described here for Webpack 4 and 5) or bundler shims (such as this one for Vite). You can simply install @react-pdf/renderer and start using it right away, the way it should be.
Fixes #1317, fixes #1645, fixes #1899, fixes #1847, fixes #1886, fixes #1744, fixes #1670, fixes #1669, fixes #1915, fixes #1786, fixes #1944, fixes #1966
To acheive this, I had to bundle some commonjs dependencies with the browser versions of react-pdf:
blob-streamrestructuredfa(was already bundled before)vite-compatible-readable-streambrowserify-zlibThis was necessary because these packages are in commonjs format (instead of ES6 module format) and will be misinterpreted when using a modern ES6 module bundler.
(Fun fact, I actually did the whole thing twice, but the first time around I learned so much about rollup that I decided it was worth doing a second time because the resulting rollup configs would be much cleaner 😆)
While I was at it, I also updated the restructure package to the latest version, which has been requested in #1451 for a long time now.
TODO: