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

Replace Deflater with pako #2944

Merged
merged 16 commits into from Dec 7, 2020
Merged

Replace Deflater with pako #2944

merged 16 commits into from Dec 7, 2020

Conversation

markotaht
Copy link
Contributor

This should fix issue #2904 .

@MrRio
Copy link
Member

MrRio commented Oct 5, 2020

Oh thanks! I think this fails in Travis as the reference PDFs need checking/updating

Copy link
Collaborator

@HackbrettXXX HackbrettXXX left a 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
Comment on lines 1 to 7
var pako;

(function() {
pako = require("pako");
})();

export { pako};
Copy link
Collaborator

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
Comment on lines 29 to 31
"peerDependencies": {
"pako": "^1.0.11"
},
Copy link
Collaborator

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
Comment on lines 26 to 28
"peerDependencies": {
"pako": "^1.0.11"
},
Copy link
Collaborator

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.

@markotaht
Copy link
Contributor Author

  1. Can i get the URL or Doc of the Deflater.js libari. The PNG support does some weird stuff with deflater, im not sure what is happening there.
  2. Trying to fix tests, but the comparison keeps failing. I manually checked the bytes of the compression outcome and the one in the expected results. The are exactly the same but the comparison still fails.

@HackbrettXXX
Copy link
Collaborator

  1. I think the deflater was taken from here but there is no additional documentation there, either. What do you mean by "weird" stuff? As far as I can see, it's simply calling the deflater with the bytes and then flushes it for some additional bytes. These bytes are concatenated with the header and the checksum.
  2. Mmh, that's odd. Maybe the way the comparison works is an issue?
  expect(actual.replace(/[\r]/g, "").split("\n")).toEqual(
    expected.replace(/[\r]/g, "").split("\n")
  );

@markotaht
Copy link
Contributor Author

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.

@HackbrettXXX
Copy link
Collaborator

Thanks very much @markotaht for the great help! I will take over from here and fix the unit tests ;)

@markotaht
Copy link
Contributor Author

Hows it going? I hope its not too difficult :D.

@HackbrettXXX
Copy link
Collaborator

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.

@owenl131
Copy link
Contributor

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

@HackbrettXXX
Copy link
Collaborator

@owenl131 ah thanks, didn't know about that ;)

This was referenced Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants