External QRBill class

[danielpanero]

Related to #385

TODO

• [x] Converting remaining SVG paths to PDF paths to increase performance
• [x] Caching the QRCode segment
• [x] Deciding on the functions / data exposed
• [x] Update documentation
• [x] Add more tests

Functions / data exposed

As I wrote in the source code, to maintain back-compatibility the class PDF_ still exposes _data through a getter and a setter. For now, I marked them as deprecated. Furthermore, in order to change data, I added a new method called changeBill. Finally I think we should give the possibility of initializing the PDF_ class without having a bill, so even if the validation steps failed, the PDF is still created.

Tests

I don't know what you are using internally to test the correctness of the PDFs created... So if you want we can implement something similar to what I use in my projects:

const testPDF = (stream: WriteStream, filename: string) => {
    return new Promise((resolve, rejects) => {
        stream.on("finish", () => {
            exec(`diff-pdf ${__dirname}/tmp/${filename} ${__dirname}/snapshots/${filename}`, (error, stdout, stderr) => {
                if (error) {
                    switch (error.code) {
                        case 1:
                            rejects(`tmp/${filename} doesn't corresponds to snapshots/${filename}`)
                            break
                        default:
                            rejects(error.message)
                            break
                    }
                }
                resolve(true)
            })
        })
    })
}

which itself utilizes diff-pdf.

[schoero]

As for the tests: I do manual tests. This is something I've wanted to improve for a long time, but never got around to it.

I think the use of diff-pdf would be a great enhancement. In addition to that I wanted to implement unit tests, especially for functions like generateQRCode.

I actually have a v4 planned, where I wanted to improve lots of internal code, as well as automate tasks like tests, versioning, releases, changelog and documentation.

But I won't find the time in the next few months to finish it.

So for now, I would suggest to either just do manual testing, or create a separate PR for that if you want to improve testing too.

[danielpanero]

Voilà, I added two tests/examples on how to use the class QRBill. Should I start updating the documentation?

[schoero]

Yes, that would be great. Let me know when I can review it.

[danielpanero]

Upon further investigation, although in this manner it is back-compatible, I don't like the syntax too much, so... what I would propose is to ship it as a breaking update and enforce a better syntax, where the goal of each class is clear and separated:

• QRBill contains all the information needed to draw the QRSlip on any instance of PDFKit.Document
• PDF_ superset of PDFKit.Document and completely detached from any QRSlip, so... the new syntax would be something like this:

const bill1 = new SwissQRBill.QRBill({...}, {...})
// In the case a bill is present when initiating the class, it will be auto-drawn and the third parameter is the same as autoGenerate
const doc = SwissQRBill.PDF("test.pdf", bill1, true)

// Therefore also something like this would be possible
const doc = SwissQRBill.PDF("test2.pdf")
const bill2 = SwissQRBill.QRBill({...}, {...})
const bill3 = SwissQRBill.QRBill({...}, {...})

doc.addQRBill(bill2);
...
doc.addQRBill(bill3);

Let me know what you think :+1:

[schoero]

Thank you for your work so far. I very much appreciate your efforts to improve this library and I like that you bring a different perspective into it.

I like the new syntax that you propose, but I don't like breaking changes 😄.

Your proposed syntax is more flexible but I think we can keep the old syntax around by the use of a constructor overload, deprecate the old syntax and change the documentation to the new syntax.

That gives the users a bit more time to migrate and I don't see any issues implementing it.

Also: I don't think we need the first variant as we no longer have a single constructor call for the complete bill anyway:

const bill1 = new SwissQRBill.QRBill({...}, {...})
const doc = SwissQRBill.PDF("test.pdf", bill1, true)`

And go just for the second one:

const doc = SwissQRBill.PDF("test2.pdf")
const bill2 = SwissQRBill.QRBill({...}, {...})
const bill3 = SwissQRBill.QRBill({...}, {...})
doc.addQRBill(bill2);
doc.addQRBill(bill3);

[danielpanero]

I implemented the new syntax and it should be completely back-compatible... There is only one small breaking change, as it was already a little bit of a mess, I switched the order of the parameters in the constructor

// Before (Browser)
constructor(data: Data, writeableStream: IBlobStream, options?: PDFOptions, callback?: Function);
constructor(data: Data, writeableStream: IBlobStream, callback?: Function){};

// After (Browser)
constructor(writableStream: IBlobStream, data?: Data, options?: PDFOptions);
constructor(writableStream: IBlobStream, callback?: Function, data?: Data, options?: PDFOptions);

// Before (Node)
constructor(data: Data, outputPath: string, options?: PDFOptions, callback?: Function);
constructor(data: Data, writableStream: Writable, options?: PDFOptions, callback?: Function);

// After (Node)
constructor(outputPath: string, callback?: Function, data?: Data, options?: PDFOptions);
constructor(writableStream: Writable, callback?: Function, data?: Data, options?: PDFOptions);

constructor(outputPath: string, data?: Data, options?: PDFOptions);
constructor(writableStream: Writable, data?: Data, options?: PDFOptions);///

So when we remove the deprecated code, it is already right:

// Browser
constructor(writableStream: IBlobStream, callback?: Function);

// Node
constructor(outputPath: string, callback?: Function);
constructor(writableStream: Writable, callback?: Function);

For now, I updated the all the tests, but they utilize the old syntax let me know, which upgrade and which not. Furthermore before updating the examples and how-to-create-a-complete-bill.md, let me know if I should change something else in the code, so I don't rewrite twice the same thing :grin:

[schoero]

I wouldn't switch the parameters. Even a small breaking change is a breaking change and requires (in this case all) users to change their implementation.

My intention is to have backwards compatibility for now, but warn users that things will change. I already have some breaking changes in my v4 branch where I improved the addTable method a bit. For the v4 release, I would like to include that, as well as my previously mentioned refactorings and build improvements, so that I can combine all breaking changes in one update and therefore users don't have to adapt their code twice.

Another thing I'd like to do is to also extract the addTable method into its own class in a similar manner you did with the QRBill class. I don't know if it is possible, but it would be great if it could also be attached to any PDFKit.PDFDocument instance.

When this is all done, we have a clear separation of concerns and a much more powerful library with a consistent API.

The problem is that I currently have limited time to work on this and I it will probably take me a few months to finish these other ideas. If we have backwards compatibility, we can release your improvements sooner.

[schoero]

Another way of dealing with this is to just publish a v4 beta or next version on npm. That way we don't have to rush out a backwards compatible release right now and you as well as anyone else can still use it already.

What do you think?

[danielpanero]

I think that would be great (publish it as next), as we don't have to care about back-compatibility and run into strange issues... what should I rebase from your last commit on v4, or do the contrary (rebase v4 from my last commit)?

As for addTable, in my code, I wrote it completely with a functional approach function addTable(doc: PDFKit.Document, table: PDFTable), but we could go also for class-based approach... I don't know maybe I have PTSD from my computer science courses in OOP in c++ :nauseated_face:

[schoero]

I can understand your trauma 😄 But I would like to still follow the principles from PDFKit. Take a look at the linearGradient:

// Create a linear gradient
let grad = doc.linearGradient(50, 0, 150, 100);
grad.stop(0, 'green')
    .stop(1, 'red');

doc.rect(50, 0, 100, 100);
doc.fill(grad);

doc.linearGradient(...) creates just an instance of a class.

If we translate that to table, the API would look something like this:

let table = doc.table(x: number, y: number, rows?: TableRow[]);
let row = table.addRow();
let column = row.addColumn();

And the table class itself would be similar to the QRBill class. I'm not sure if that is the best way for tables, or if it's even possible with that syntax, because the table automatically handles width and height as well as page overflow.

But let's not focus too much on tables, this is something I can try out later.

I created a next branch, you can target your PR at that branch. So as the next steps, you can remove the compatibility overloads etc and implement the API you think is best. Btw. I think the switch of the order of the parameters makes total sense.

I can do a review of that by the end of the week.

[danielpanero]

Top top :+1: