Skip to content
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.

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

React-PDF Unusable For Modern Packaging Systems & Server Side Rendering #2624

Open
jd-carroll opened this issue Feb 7, 2024 · 10 comments
Open

Comments

@jd-carroll
Copy link

There are multiple issues here, please read carefully. This should probably be broken into multiple issues but consolidating here to keep it simple for me.

The issues described here will also resolve: #2599 & #2623

React-PDF Unusable for Modern Packaging Systems

This one is a little frustrating, please take some time to read the documentation for how ESM modules work:
https://nodejs.org/api/packages.html#package-entry-points

TLDR;
The current exports of @react-pdf/renderer has no browser entry making it impossible for a project which respects ESM modules to work in the browser.

Additionally, if you're going to bundle both CommonJS and ES Module files in the same package, please use .mjs file extensions. Setting "type": "module" in the package isn't exactly accurate because the package could be used as a CommonJS package and not just an ES Module. More importantly, if you're going to continue to use .js file extensions, please reserve that for the CommonJS files.

Here is what I think you should target in @react-pdf/renderer/package.json

  // This is an exception to not using .js for ES modules, but this could be phased out over time
  // If the bundling system actually respects ES Modules it will completely ignore this field
  // and instead use "exports" entry
  // Note: This file is identical to ./lib/react-pdf.modern.mjs
  "module": "./lib/react-pdf.legacy-esm.js", 
  "main": "./lib/cjs/react-pdf.js",
  "types": "./lib/index.d.ts",
  "sideEffects": false, // (hopefully?)
  "exports": {
    ".": {
      "types": "./lib/index.d.ts",
      "node": {
        "import": "./lib/react-pdf.modern.mjs",
        "default": "./lib/cjs/react-pdf.js"
      },
      "browser": {
        "import": "./lib/react-pdf.browser.modern.mjs",
        "default": "./lib/cjs/react-pdf.browser.js"
      },
      "default": {
        "import": "./lib/react-pdf.modern.mjs",
        "default": "./lib/cjs/react-pdf.js"
      }
    }
  },

Note: While I only documented it for @react-pdf/renderer, this would be required for every package.

React-PDF Unusable for Server Side Rendering

This one is a little more complicated and will require actual changes in the code.

In short, when using a server side rendering framework like NextJS your code is run twice, once on the server to generate a (typically) reusable [pre]rendered page which is served for incoming requests. The second is in the browser where the code is actually running for its intended purpose.

If you look at the generated output from a NextJS build, you will actually see two different bundles:
image

  1. server/chunks -> Used for running on the server to generate the pre-rendered page
  2. static/chunks -> Served to the client for running in the browser

The problem

When NextJS builds the bundle for server/chunks it uses the default conditions of ["node", "import"]

Therefore the server side bundle will always receive the node version of the package causing legitimate uses (eg. usePDF) of the package to break.

To be honest, it also seems to me that you are abusing the "browser" convention of bundlers. You are not providing an "alternate" implementation targeted for the browser environment, you are providing a completely different implementation.

Further, if I wanted to render the PDF to a stream or buffer on the browser, why are you preventing me? If I needed to send the PDF to the server, that is exactly what I would look to do.

Work Arounds

If you are landing on this issue and think it pertains to you, there are no work arounds.

Your only solution (for now) is specifying resolutions for all packages under @react-pdf

  "resolutions": {
    "@react-pdf/fns": "2.0.1",
    "@react-pdf/font": "2.3.7",
    "@react-pdf/image": "2.2.2",
    "@react-pdf/layout": "3.6.3",
    "@react-pdf/pdfkit": "3.0.2",
    "@react-pdf/png-js": "2.2.0",
    "@react-pdf/primitives": "3.0.1",
    "@react-pdf/render": "3.2.7",
    "@react-pdf/renderer": "3.1.14",
    "@react-pdf/stylesheet": "4.1.8",
    "@react-pdf/textkit": "4.2.0",
    "@react-pdf/types": "2.3.4",
    "@react-pdf/yoga": "4.1.2"
  },

Final thought:
diegomura wojtekmaj - Thank you for creating this module, it is actually pretty useful. All criticism with ❤️

@wojtekmaj
Copy link
Contributor

The current exports of @react-pdf/renderer has no browser entry making it impossible for a project which respects ESM modules to work in the browser.

Good point. We should have an additional browser and exports.*.browser entries in package.json files.

Additionally, if you're going to bundle both CommonJS and ES Module files in the same package, please use .mjs file extensions. Setting "type": "module" in the package isn't exactly accurate because the package could be used as a CommonJS package and not just an ES Module.

Huh? Well, setting "type": "commonjs" is also not accurate because the package could be used as ESM. "type": "module" tells tools like bundlers and Node.js how .js files should be treated. And all files except for .cjs ones are ESM. So with my current state of knowledge I'd disagree with you.

More importantly, if you're going to continue to use .js file extensions, please reserve that for the CommonJS files.

See above. I don't think that should be necessary.

Regarding Next.js problem - starting with Next.js 14.1.1, on App Router, I've seen no issues with PDF rendering, so that's a good start I think.

Therefore the server side bundle will always receive the node version of the package causing legitimate uses (eg. usePDF) of the package to break.

Yeah, I agree, that's a complicated part. How we could provide usePDF for Next.js SSR?

@jd-carroll
Copy link
Author

jd-carroll commented Feb 7, 2024

Well, setting "type": "commonjs" is also not accurate

Completely, I don't think "type" should be there at all, but it is important to still provide "types" for environments that understand none of the new ES Module system.

As an example, here is what @redux/toolkit sets as there exports:
https://github.com/reduxjs/redux-toolkit/blob/f75f185adbfd90bce880122dd3b856f37aec6ee5/packages/toolkit/package.json#L25-L34
(You'll notice a similar pattern to my comment above. I've spent more time than I should have going through how they arrived at their solution and reading specs defining the new ES Modules.)

And all files except for .cjs ones are ESM

Yes, but that isn't the issue. For environments that have no understanding of ES Modules they could still attempt to load the .js files which would lead to syntax errors. So while it is more of a style thing, putting the new ES Module code into .mjs files provides a more durable implementation for consumers.

How we could provide usePDF for Next.js SSR?

I think it is more a question of what are you protecting against? If you purely look at usePDF there is no reason to have a "browser" vs "node" implementation of it.

What I didn't include above is that in addition to the resolutions I had to patch@react-pdf/renderer (using yarn) where I exactly copied the usePDF implementation into the node version form its browser counter part. (And it works without issue)

So I would do two things:

  • Remove the renderToFile API in the node implementation. Anyone who uses that API can instead renderToStream and then pipe the stream to a file themselves
  • Collapse the browser and node implementations into a single implementation

I look at the two implementations and I don't see anything that isn't available in both environments. Maybe change window references to globalThis and check for undefined. But at that point, some is probably doing something they shouldn't so an error I think is appropriate.

There could be other aspects that I am missing which may prevent this, and this is exclusively related to @react-pdf/renderer, but I don't see a reason to keep separate browser & node implementations.

@diegomura
Copy link
Owner

diegomura commented Feb 9, 2024

Thanks @wojtekmaj and @jd-carroll for all the thoughts here!

Criticisms were taken with ❤️. My only comment, if I might, is that the "unusable" part of the issue title is a bit unfair as I'm running current versions with latest vite on the examples package, and NextJS for the docs website 😄

Having said that haha, let me comment bit by bit

The current exports of @react-pdf/renderer has no browser entry making it impossible for a project which respects ESM modules to work in the browser.

Agree, but after reading docs, shouldn't be more like this?

"exports": {
    ".": {
      "types": "./lib/index.d.ts",
      "node": {
        "import": "./lib/react-pdf.modern.mjs",
        "default": "./lib/cjs/react-pdf.js"
      },
      "import": "./lib/react-pdf.browser.modern.mjs",
      "default": "./lib/cjs/react-pdf.browser.js"
    }
  },

browser as the docs says is a community condition definition and not really part of the standard (even though it works). Based on what the docs says that above should work because node takes precedence. Wouldn't this be more future proof and also simpler?

Additionally, if you're going to bundle both CommonJS and ES Module files in the same package, please use .mjs file extensions. Setting "type": "module" in the package isn't exactly accurate because the package could be used as a CommonJS package and not just an ES Module.

I'm with @wojtekmaj here. I don't really see how it changes considering conditional exports are in place.

From docs: The preceding example uses explicit extensions .mjs and .cjs. If your files use the .js extension, "type": "module" will cause such files to be treated as ES modules, just as "type": "commonjs" would cause them to be treated as CommonJS

So I rather have them as type: "module" which is what both documented examples in node docs here recommend doing

re/ SSR

I'm very much aware of the issues here. It's just something I never had time thinking and working on. But it's still possible to use it by disabling SSR around the react-pdf components as the REPL does.

Tbh SSR PDFs using usePDF not sure makes sense. I don't really know how you would rehydrate a PDF generated on the server and then re-rendered in UI. If we ever fix this imo it should do nothing on the server and just generate the PDF on the client. Does that make sense to you guys?

About points above:

  • Remove the renderToFile API in the node implementation. Anyone who uses that API can instead renderToStream and then pipe the stream to a file themselves
    • Can definitely do that, but not sure I see how it would fix anything
  • Collapse the browser and node implementations into a single implementation
    • At my eyes this is not very straightforward. React-pdf code is pretty isomorphic, but their dependencies aren't. Lot's of them depends on fs or zlib or other node dependencies that we either ignore and alter code on browser builds, or need to replace by polyfill imports.

@jferrettiboke
Copy link

Regarding Next.js problem - starting with Next.js 14.1.1, on App Router, I've seen no issues with PDF rendering, so that's a good start I think.

@wojtekmaj 14.1.1 isn't even out yet; it's on Canary. I've installed the latest Canary version, which is 14.1.1-canary.77, and it doesn't work.

@mleboulaire
Copy link

mleboulaire commented Feb 29, 2024

I tried something like below because the version 3.1.14 of renderer doesn't contain a fix we need, but I can't make 3.3.8 work. Is it impossible for now ?

"overrides": {
    "@react-pdf/fns": "2.2.1",
    "@react-pdf/font": "2.4.4",
    "@react-pdf/image": "2.3.4",
    "@react-pdf/layout": "3.11.1",
    "@react-pdf/pdfkit": "3.1.5",
    "@react-pdf/png-js": "2.3.1",
    "@react-pdf/primitives": "3.0.1",
    "@react-pdf/render": "3.4.3",
    "@react-pdf/renderer": "3.3.8",
    "@react-pdf/stylesheet": "4.2.4",
    "@react-pdf/textkit": "4.4.1",
    "@react-pdf/types": "2.3.4",
    "@react-pdf/yoga": "4.1.2"
  }

Still having errors like node_modules/@react-pdf/renderer/lib/react-pdf.d.cts:635:3 - error TS2323: Cannot redeclare exported variable 'PDFRenderer'. On Node.js 20.11.1.

@BeckmannArmin
Copy link

Are there any udpates here?

@Kluckmuck
Copy link

Bump, would love to see this fixed!

@theskillwithin
Copy link

bump

@colinamicon
Copy link

colinamicon commented Sep 6, 2024

we ran into an issue upgrading to 9.10 in our React/GraphQL app

Here was our fix in config-overrides.js webpack export:

module.exports = { 
... 
        // add mjs to webpack
        config.module.rules.push({
            test: /\.(js|mjs)$/,
            type: 'javascript/auto',
        });

...
}

if anyone has a better idea or thoughts lmk

@JosephCanete
Copy link

JosephCanete commented Oct 1, 2024

Bump for the fix please curious to know 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants