-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Replace Deflater with pako #2944
Conversation
Oh thanks! I think this fails in Travis as the reference PDFs need checking/updating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR ;)
- Please revert the changes to the files in the
dist
folder. These files are only updated on release. - Could you also have a look at the usages in the
png_support
file and replace them with pako? Would be awesome :) - Please update the reference PDF files for the broken tests
- We need to come up with something so
npm run test-unit
will still work. The issue is that the browser cannot resolve the pako import directly. We either need to add a preprocessor (e.g. webpack) or need to pre-build the pako package and add a "custom resolver" to the karma config. If you don't feel comfortable about that, I can give it a try ;)
src/libs/pako.js
Outdated
var pako; | ||
|
||
(function() { | ||
pako = require("pako"); | ||
})(); | ||
|
||
export { pako}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just use import pako from "pako"
directly in the filters file? AFAIK Rollup should be able to cope with that.
package.json
Outdated
"peerDependencies": { | ||
"pako": "^1.0.11" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pako should be a regular dependency, not a peer dependency.
bower.json
Outdated
"peerDependencies": { | ||
"pako": "^1.0.11" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pako should be a regular dependency, not a peer dependency.
|
|
# Conflicts: # src/modules/filters.js
So i was able to fix the test. And convert PNG to pako also. Messed around with karma abit. whenn added pako.js location to karma.config.js under files: then some tests seemd to pass or something, but the main error i got was that jsPDF is undefined. So im not fully sure what i going on. So if you dont mind copuld you look into it :D. |
Thanks very much @markotaht for the great help! I will take over from here and fix the unit tests ;) |
Hows it going? I hope its not too difficult :D. |
Didn't have time to look into this so far. Do you need this PR for the Hacktoberfest? Otherwise I will probably only get to it next week. |
If you need the PR for hacktoberfest you can tag it as hacktoberfest-accepted, then it will be counted even if it hasn't been merged yet. Then theres no need to rush through it |
@owenl131 ah thanks, didn't know about that ;) |
This should fix issue #2904 .