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

Can't open documents generated with ^8.1.0 in Next.js API route using Packer.toBuffer() #2237

Closed
rbourdon opened this issue Jul 14, 2023 · 7 comments

Comments

@rbourdon
Copy link

I use docx in a Next.js application (pages directory) in an api route with Packer.toBuffer(), and after v8.1.0 Word has failed to open the generated document.

Here's a simple reproduction on codesandbox: https://codesandbox.io/p/sandbox/exciting-meadow-k89f6g

Reverting back to 8.0.4 resolves the issue.

@SawkaDev
Copy link

Same Issue here.

@joshkel
Copy link

joshkel commented Jul 18, 2023

I've encountered what I believe is the same issue. I have a Hapi backend that started returning unusable values when I upgraded to 8.1.0.

Since it started with 8.1.0, I believe it's the result of the migration to Vite in #2148.

From what I can tell:

The problem arises because the output of Packet.toBuffer() isn't a Node.js Buffer. It calls itself a Buffer (e.g., if you call JSON.stringify on it, "Buffer" appears in the output), and it's an instance of Uint8Array (just like a real Node.js Buffer), but it's not an instance of Buffer, so Hapi does not invoke its code path for serving a binary blob. I haven't looked into Next.js, but I'm guessing it's the same root problem.

From inspecting the build/index.cjs build of docx, it looks like docx -> jszip -> Buffer is resolving to the third-party buffer package instead of using Node.js's native Buffer support. (In case it helps, my application is using the build/index.cjs build of docx.)

@dolanmiu
Copy link
Owner

I upgraded the project from webpack to vite for better DevX and native ESM support

Yes, this is Vite doing some of it's own optimisations (I am guessing to make it compatiable for web and node etc).

Still looking into it

joshkel added a commit to joshkel/docx that referenced this issue Jul 19, 2023
Instead of bundling all dependencies into docx's built JS (which is a common approach for browser environments), this lets them be imported through import / require (as is typical with NPM).  This reduces the size of docx's built files from ~711kB to ~250kB, avoids duplicating code for applications that are already using dependencies such as jszip.  It also avoids any Node.js polyfills, which is a mixed bag - that prevents problems such as dolanmiu#2237 but leaves it up to users to provide polyfills themselves if using a browser environment.

Code for listing dependencies is based on https://stackoverflow.com/a/74578129/25507
joshkel added a commit to joshkel/docx that referenced this issue Jul 19, 2023
Instead of bundling all dependencies into docx's built JS (which is a common approach for browser environments), this lets them be imported through import / require (as is typical with NPM).  This reduces the size of docx's builds from ~711kB each to ~250kB each and avoids duplicating code for applications that are already using dependencies such as jszip.  It also avoids any Node.js polyfills, which is a mixed bag - that prevents problems such as dolanmiu#2237 but leaves it up to users to provide polyfills themselves if using a browser environment.

Code for listing dependencies is based on https://stackoverflow.com/a/74578129/25507
@joshkel
Copy link

joshkel commented Jul 19, 2023

Looks like the newly released 8.2.1 fixes this. (See #2222.) Thanks, @dolanmiu!

I'm not an expert on this (I've never published to NPM), but it seems to me that it would be better for docx to not bundle its dependencies at all? That would reduce its bundle size and prevent issues like this (although it's possible that it might make users responsible for providing dependencies themselves). See #2254.

@SawkaDev
Copy link

8.2.1 Fixed it for me as well.

@rbourdon
Copy link
Author

Confirming this is resolved with 8.2.1. Thanks @dolanmiu!

@dolanmiu
Copy link
Owner

Excellent, thanks for the testing everyone!

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

No branches or pull requests

4 participants