vbuch / node-signpdf

Simple signing of PDFs in node.
MIT License
719 stars 178 forks source link

allow pdfkitAddPlaceholder to receive custom location, contactInfo, name #87

Closed ashirman closed 4 years ago

ashirman commented 4 years ago

currently pdfkitAddPlaceholder API allows to customise only reason field for generated signature.

Makes sense to extend it so we could have chance to set also contactInfo, name, location fields of signature. Changes are backward compatible so users do not need to explicitly pass these values if they do not wish to do it.

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 317


Totals Coverage Status
Change from base Build 312: 0.0%
Covered Lines: 282
Relevant Lines: 282

💛 - Coveralls
vbuch commented 4 years ago

I'm looking on mobile but I have a small concern: those used to be instances of String and not strings. I am lrettybsure there was a good reason for that lying somewhere in pdfkit. There was some logic in there that checked if var isnatceof String. That was when this code was initially written. Not sure if things changed. Can you check that?

ashirman commented 4 years ago

Not sure if things changed. Can you check that?

Indeed, there are some places where pdfkit check instance of given strings. eg

   // String literals are converted to the PDF name type
    if (typeof object === 'string') {
      return "/".concat(object); // String objects are converted to PDF strings (UTF-16)
    } else if (object instanceof String) {

so I feel it is better to wrap passed strings as String eg ContactInfo: new String(contactInfo) so we could rely on internal behaviour of pdfkit. PR has beed updated properly please take a look.

vbuch commented 4 years ago

Would be nice if you could include tests that check if the data is actually stored in a resulting pdf.

ashirman commented 4 years ago

Would be nice if you could include tests that check if the data is actually stored in a resulting pdf.

I put couple tests to be able to check pdfkitAddPlaceholder method behaviour.

ashirman commented 4 years ago

the build has been restored and now changes are ready to be merged. Could you please do that?

vbuch commented 4 years ago

Released in 1.3.0