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: import only md5 hash from crypto-js #1607

Merged
merged 3 commits into from
Dec 18, 2021
Merged

fix: import only md5 hash from crypto-js #1607

merged 3 commits into from
Dec 18, 2021

Conversation

jeetiss
Copy link
Collaborator

@jeetiss jeetiss commented Nov 22, 2021

crypto-js contains a lot of hash algorithms and functions, but pdfkit uses only md5
I change the imports to include only md5 code to bundle, it should save ~42kb

Снимок экрана 2021-11-22 в 10 29 52

@jeetiss jeetiss requested a review from diegomura November 22, 2021 07:35
@jeetiss jeetiss self-assigned this Nov 22, 2021
@jeetiss
Copy link
Collaborator Author

jeetiss commented Nov 26, 2021

should fix #1618

@jeetiss jeetiss merged commit a592e99 into master Dec 18, 2021
@jeetiss jeetiss deleted the crypto-tweaks branch December 18, 2021 13:34
@github-actions github-actions bot mentioned this pull request Dec 18, 2021
@chris-morgan
Copy link

I’m seriously confused here, because as far as I can tell it’s just completely broken, so that AttachmentsMixin no longer works.

78bfe8c made sense: replace crypto-js import with crypto-js/md5 and crypto-js/core.

But then 849dca1 ruined everything by retreating to putting everything in crypto-js/md5, but treating it as though it exported CryptoJS, whereas in actual fact it gives you CryptoJS.MD5. So the checksum line will always throw an exception because it has the wrong object. After puzzling about it for a bit, I’m guessing the purpose of the commit was to adjust the Rollup external piece, and took the wrong approach due to some fascination with mapping. Instead, I say, just add new externals, e.g. [...Object.keys(pkg.dependencies), 'crypto-js/md5.js', 'crypto-js/core.js'].

A secondary problem is that the import paths don’t work in ECMAScript Modules: you need the file extension, so 'crypto-js/md5' should be 'crypto-js/md5.js', and 'crypto-js/core' 'crypto-js/core.js'. This change would restore this piece to working in both CommonJS and ESM. (This is related to #2068.)

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.

2 participants