diegomura / react-pdf

📄 Create PDF files using React
https://react-pdf.org
MIT License
14.39k stars 1.13k forks source link

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

Open jd-carroll opened 4 months ago

jd-carroll commented 4 months ago

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 :heart:

wojtekmaj commented 4 months ago

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 commented 4 months ago

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:

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 commented 4 months ago

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:

jferrettiboke commented 4 months ago

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.

KaaZMi commented 4 months ago

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 commented 3 days ago

Are there any udpates here?