Add high-level API for adding document JavaScripts #643
Conversation
There was a problem hiding this comment.
Hello @brodo!
Thank you very much for working on this. I'm sure many users will find this to be a useful addition to the library. I've just requested a few changes.
Please also update some of the integration and unit tests to exercise this new functionality (I'd recommend taking a look at the CONTRIBUTING.md if you haven't had a chance already).
src/api/PDFDocument.ts
Outdated
| * @param script The JavaScript to execute. | ||
| * @param name The name of the script. Must be unique per document. | ||
| */ | ||
| addJavascript(script: string, name: string) { |
There was a problem hiding this comment.
Can we alter the implementation of this method a bit? There are some existing patterns in the codebase that should be followed. Please see https://github.com/Hopding/pdf-lib/blob/master/src/api/PDFEmbeddedFile.ts#L48. File embedding is similar to JavaScript embedding in some respects.
There was a problem hiding this comment.
I've taken the code you suggested and adopted it. It's way better this way...
There was a problem hiding this comment.
Thanks! I'll try to review it again tomorrow evening.
|
I also added unit tests and integration tests for node. As pretty much no PDF viewer besides Acrobat supports this, I think the other integration tests do not make much sense. |
|
@brodo Thanks for adding the test! It's true that Acrobat is pretty much the only reader that supports JavaScript in PDFs. However, the tests for Deno, web, and React Native will still need to be updated to exercise this feature. This is because (a) The tests are meant to be exact duplicates between environments (to the greatest extent possible), this makes them much easier to update and maintain. And (b) we need to ensure that the implementation of |
|
Should be done now! |
* Add high-level API for adding document JavaScripts (#643) * Add high-level API for adding document JavaScripts * Add example to `addJavascript` documentation and switch parameter order. * Add check for existing catalog entries in `addJavascript` * Add unit tests for `addJavascript` * Add Node.js integration test for `addJavascript` * Add integration test for `addJavascript` in web * Add integration test for `addJavascript` in react native * Add integration test for `addJavascript` in deno * Cleanup PDFDocument.addJavaScript * Revert scratchpad Co-authored-by: Julian Dax <[email protected]>
|
This feature was just released in version |
* Add high-level API for adding document JavaScripts (#643) * Add high-level API for adding document JavaScripts * Add example to `addJavascript` documentation and switch parameter order. * Add check for existing catalog entries in `addJavascript` * Add unit tests for `addJavascript` * Add Node.js integration test for `addJavascript` * Add integration test for `addJavascript` in web * Add integration test for `addJavascript` in react native * Add integration test for `addJavascript` in deno * Cleanup PDFDocument.addJavaScript * Revert scratchpad Co-authored-by: Julian Dax <[email protected]>
This PR adds an API for adding JavaScript to a document. It is based on this issue).