Open jhonnyman opened 2 years ago
MohammedEssehemy
commented
2 years ago Hi, Thank you for your contribution. We are using npm for this project, so can you please retry with npm and rever the changes related to yarn.
Also for the added package, can you explain why do we need it
jhonnyman
commented
2 years ago I'll make a new PR with the yarn.lock restored.
The reason for adding ieee754 package is that it no longer bundled in recent Node.Js versions.
My current project runs on Node.Js 14 and I was not able to run the verifyPDF library if I did not added this package manually inside the project. Since I don't use this library anywhere else, it makes sense to be a dependency of this library. Maybe there are more people with this issue as well.
As we want to upgrade Node.Js to 16 for our project, bumping node-forge is required for compatibility with other libraries.
Let me know if it makes sense before I open a new PR.
jhonnyman
commented
2 years ago @MohammedEssehemy would you consider reviewing the breaking changes on node-forge@^1.2.1? There are some critical security issues solved with recent versions, however it breaks compatibility with verifyPDF library.
This is a requirement to keep some libraries up to date as projects and libraries evolve.
I you're not able to, I may dig into this issue. However I don't have the full context of the implementation. Let me know.
MohammedEssehemy
commented
2 years ago Hi, for node-forge, sure feel free to go forward and dig into it, that's the good part of open source. for yarn.lock can we please do it in the same PR, so that we don't introduce unnecessary changes. for ieee754 it's available already in the package.jsob inside https://github.com/ninja-labs-tech/verify-pdf/blob/89d8b1b8078e3e35a90de970866b03bb13d856e8/packages/buffer/package.json#L7, you can upgrade it there if needed. Maybe yarn didn't interpret it correctly, but we don't need to add it to the top level package.json since it's not used directly by the package.
dima-dmv
commented
1 year ago Hello @jhonnyman @MohammedEssehemy Are you ok to merge this? I'm having similar issues as a topic starter so It'd be beneficial to have it merged. Thanks for the great service!
jhonnyman
commented
1 year ago Hello all. I apologise for pushing this merge before. The thing is I needed a major version upgrade node-forge that had breaking changes. As I could not dedicate the time to properly address this upgrade, I would like to close this PR as it does not make sense
In my project we require bumping node-forge dependencies to at least 1.2.0. From 1.2.0 there are breaking changes that should be addressed due to high vulnerabilities. For now bumping node-forge version can solve my issue. Also added the package ieee754 as dependency as recent Node.js versions don't have it globally available.