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

Fix pattern support possibly broken by minification #168

Merged
merged 1 commit into from
Aug 30, 2022

Conversation

perliedman
Copy link
Contributor

@perliedman perliedman commented Jul 24, 2022

As noted in #137, patterns are sometimes not correctly converted from SVG to PDF: in these cases, patterned areas turn into black solid fill. The issue occurs for example when using production mode from a Create React App configuration.

Digging into this, I found that the generation of PDF patterns relies on a string comparison with the constructor name:

if (color[0].constructor.name === 'PDFPattern') {

But the constructor in question belongs to a private type inside another function:

let pattern = new (function PDFPattern() {})();

At least some minifiers are smart enough to realize this name could be removed, causing the constructor name comparison to fail.

I have addressed this by replacing the private class/constructor with a simple object and a type property that I set to PDFPattern. This resolves the issue and I think it works equally well.

If types/classes are preferred, I guess we could make PDFPattern a class known throughout the library and replace the constructor name check with an instanceof check.

Constructor name can be lost in minification. Fixes alafr#137.
@perliedman
Copy link
Contributor Author

Did not realize until after I posted this that #159 seems to address the same issue (but with a bit of a complicated diff...).

@mickey58github
Copy link

@perliedman Hi Per, I'm facing this problem in our app. Strangely, it works fine in the dev build, but shows these black fills with the production build, on Heroku cloud. Saw another issue that says it has to do with minified js being used in production...

The problem affects rectangles that use pattern fills through style = "fill: url(#my-pattern-id)". Those are rendered black using the production build.

Can you provide a fix for this issue? It is a blocker for the usage of pdfSave in our app.

@perliedman
Copy link
Contributor Author

@mickey58github this PR is a fix to the issue, so you can fix it by applying this patch.

For my personal projects, I currently refer to my GitHub fork of the project like this while waiting for this PR to be merged: https://github.com/perliedman/o-scout/blob/master/package.json#L27 - it's an easy fix, but don't do it for anything really important :)

@mickey58github
Copy link

@perliedman - Per, thanks for responding. I'm a bit afraid of forking my own version. I need reliable PDF generation.

Meanwhile, I worked around the problem by moving from PDFSave to PDFKitTable - though that required me to move my PDF generation code from browser-side to Node server-side. So far, that works fine, but I haven't done complex PDFs yet with it.

@mickey58github
Copy link

mickey58github commented Aug 30, 2022

@perliedman - Per, sorry for my ignorance, but I'm unsure how to leverage this fix. It looks like it is a fix to svg-to-pdfkit, but my package.json (for my client/browser side code which has the PDFsave "black rectangles" pattern problem) shows only "pdfmake": "^0.2.5", no svg-to-pdfkit.

My package.json on the server, which uses PDFKit, not PDFSave, shows: pdfkit-table": "^0.1.99", "svg-to-pdfkit": "^0.1.8" - but that is unrelated to the PDFSave "black rectangles" pattern problem....

@grumd
Copy link

grumd commented Aug 31, 2022

Do you guys know if this is going to be released to npm? Last release was 3 years ago.

@mickey58github
Copy link

Do you guys know if this is going to be released to npm? Last release was 3 years ago.

You're right, it seems this stuff lives on Github only, while I assumed I get a fixed version through NPM.

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

Successfully merging this pull request may close these issues.

None yet

4 participants