-
Notifications
You must be signed in to change notification settings - Fork 38
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
refactor(qr): replace jsdom and d3 with cheerio #1286
Conversation
Do you have any metrics to back up this PR? |
As of now i haven’t evaluated the metrics, but potentially they would be the application load speed and the qr code generation speed |
Update: I realised I can't evaluate the application load speed as the files are stored locally on my device, however, I checked the bundle sizes for cheerio, jsdom and d3 on Bundlephobia and found that cheerio's bundle footprint is about 323.8kB minified, while jsdom and d3 are 2.9MB and 278.8kB minified respectively. After gzipping, cheerio is at 88.5kB while jsdom and d3 are 726kB and 86.7kB respectively. Hence, there should theoretically be a 89% reduction in the bundle footprint for this code segment specifically. |
Actually, there is zero change in bundle size because the QR code service is on the server. |
@liangyuanruo another reason to replace jsdom and d3 is that of dependency management - cheerio is lighter and has fewer dependencies than jsdom+d3 (7 vs 23 + 30), and despite having a limited API relative to d3, is still suitable enough for our usecase of generating SVGs. Given that we only use a tiny subset of the incumbents (ie, jsdom to create a DOM and d3.select to manipulate the DOM), it makes sense to have the libraries replaced, so that generally, we minimise the number of dependency updates that we may get but not need, particularly from d3. |
Still doesn't sound like a very strong reason to change battle-tested server-side code, but if you insist... I see that we have unit tests in |
btw can help to remove the unnecessary typecast in the unit tests in const buffer = (await qrCodeService.createGoQrCode(
testUrl,
ImageFormat.PNG,
)) as Buffer |
@orbitalsqwib can get to that once he's subjected the codebase to k6 testing |
sure thing, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See this and the other request to remove the test
QR code generation speed metric:100 virtual users Before refactor: After refactor: Memory Heap usage metric:100 virtual users Before refactor: After refactor: Noted that the profiler for before refactor ran 2 minutes longer than the after refactor due to the total runtime for before refactor being longer, thus the after refactor's heap allocation may be underestimated. |
7d9fc60
to
9d7e710
Compare
077d2e7
to
482683e
Compare
482683e
to
47954da
Compare
Problem
Currently, the jsdom and d3 libraries have been imported to generate HTML for the QRCodeService module.
However, we only use a small subset of the functionalities contained within these libraries.
Solution
Replaced jsdom and d3 libraries with cheerio to reduce client app memory footprint.
Closes #1284.
New dependencies:
cheerio
: Lightweight markup parser that can traverse and manipulate HTML.New dev dependencies:
@types/cheerio
: Supplies types for cheerio