[Reporting/Tech Debt] Convert PdfMaker class to TypeScript #81242

tsullivan · 2020-10-20T20:40:40Z

Summary

Preliminary cleanup and work on tech debt for #78726

Problem: Canvas integrates with Reporting using the preserve_layout option for PDF. This is not ideal as Canvas pages themselves are the only layout that the user wants in the report.

In a future PR, we will add a new Layout class for Canvas that will just use the image as the page contents, instead of a layout with a header, margins, and a footer.

Until we can do that, this PR is needed to take care of tech debt on the classes that are involved with generating a PDF using a Layout.

Cleanups:

Broke up x-pack/plugins/reporting/server/export_types/printable_pdf/lib/pdf/index.js by moving helper functions to separate files
Converted PdfMaker to TypeScript
Removed a pointless factory function for constructing the PdfMaker instance
Added a dependency of @types/pdfmake to ensure our objects are structured with the right interface
Fixed the structure of an object that didn't match the expected interface
Searched for some any usage and fixed it.
Expanded on Layout definitions to account for the way PdfMaker is using the layout objects
Added a comment that explains where the default logo gets imported

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

tsullivan · 2020-10-20T23:16:34Z

x-pack/plugins/reporting/server/export_types/printable_pdf/lib/pdf/get_tempate.ts

+        margin: [0, 0, subheadingMarginBottom, 20],
+      },
+      warning: {
+        color: '#f39c12', // same as @brand-warning in Kibana colors.less


Wow, this comment is a bit outdated.

Will leave it as-is until it becomes an issue.

tsullivan · 2020-10-21T04:08:22Z

x-pack/plugins/reporting/server/export_types/printable_pdf/lib/pdf/get_tempate.ts

+        alignment: 'left',
+        fontSize: headingFontSize,
+        bold: true,
+        margin: [headingMarginTop, 0, headingMarginBottom, 0],


Compare to

kibana/x-pack/plugins/reporting/server/export_types/printable_pdf/lib/pdf/index.js

Lines 268 to 269 in a74cea3

marginTop: headingMarginTop,

marginBottom: headingMarginBottom,

The previous code used invalid properties: marginTop, marginBottom

Looks like the margin(Left|Top|Right|Bottom) is supported in the code, which means the previous code should be expected to work. So changing to margin: [x, y, z, q] should lead to the same result.

https://github.com/bpampuch/pdfmake/blob/b4b4906f7db27a78b33051cd90410aec4ec73478/tests/unit/DocMeasure.spec.js#L502

tsullivan · 2020-10-21T04:08:53Z

x-pack/plugins/reporting/server/export_types/printable_pdf/lib/pdf/get_tempate.ts

+        alignment: 'left',
+        fontSize: subheadingFontSize,
+        italics: true,
+        margin: [0, 0, subheadingMarginBottom, 20],


The previous code used ~~invalid~~ deprecated properties: marginBottom, marginLeft

kibana/x-pack/plugins/reporting/server/export_types/printable_pdf/lib/pdf/index.js

Lines 275 to 276 in a74cea3

marginLeft: 20,

marginBottom: subheadingMarginBottom,

tsullivan · 2020-10-21T04:10:14Z

x-pack/plugins/reporting/server/export_types/printable_pdf/lib/pdf/index.ts

+
+  getBuffer(): Promise<Buffer | null> {
+    return new Promise((resolve, reject) => {
+      if (!this._pdfDoc) {


Note: the previous code did the undefined check outside of the Promise, which caused TS to complain about lines 143-145

kibana/x-pack/plugins/reporting/server/export_types/printable_pdf/lib/pdf/index.js

Line 138 in a74cea3

if (!this._pdfDoc) {

tsullivan · 2020-10-21T16:22:46Z

@elasticmachine merge upstream

poffdeluxe

Looks good to me. Test it out on a few different workpads and all seems good!

kibanamachine · 2020-10-21T20:48:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: f4f6bbe

Metrics [docs]

async chunks size

id	before	after	diff
`enterpriseSearch`	642.7KB	642.7KB	+27.0B

distributable file count

id	before	after	diff
`default`	48052	48055	+3

page load bundle size

id	before	after	diff
`upgradeAssistant`	65.0KB	65.0KB	+27.0B

History

💚 Build #83162 succeeded 58dd494
💚 Build #83124 succeeded 3f2c6ab
💛 Build #82982 was flaky 57b94a4
💚 Build #82961 succeeded b3743d3
💚 Build #82951 succeeded ca79e98

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…1242) * convert pdf.js to TS * more typescript * simplify caller * more typescript * more typescript * fix the code to match the expected interface * very cool comment * interface correction * remove unused class method * add unit test for PdfMaker * file rename for typo correction Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* master: (63 commits) [KP] Fix Headers timeout issue (elastic#81140) [ML] Functional tests - stabilize typing with checks service method (elastic#81338) tabify - support docs (elastic#80351) [Security Solution][Detections] Look-back time logic fix (elastic#81383) [Workplace Search] Add top-level tests for Groups (elastic#81215) [Fleet] Fix agent action observable for long polling (elastic#81376) [Maps] fix feature tooltip remains open when zoom level change hides layer (elastic#81373) skip flaky suite (elastic#78689) chore(NA): add spec-to-console and plugin-helpers as devOnly dependencies (elastic#81357) Ensure some data is returned (elastic#81375) Change dumb-init to tini (elastic#81126) [Reporting/Tech Debt] Convert PdfMaker class to TypeScript (elastic#81242) Use Storybook Controls instead of Knobs (elastic#80705) [junit] make sure that report paths are unique (elastic#81255) bump elastic/elasticsearch-js version to 7.10.0-rc1 (elastic#81288) run ssl tests on CI (elastic#81320) Fix alert defaults (elastic#81207) [ML] DF Analytics wizard: ensure user can set mml manually or select to use given estimate (elastic#81078) Add UI notifier to indicate secret fields and to remember / reenter values (elastic#80657) [Monitoring] Use async/await (elastic#81200) ...

…81400) * convert pdf.js to TS * more typescript * simplify caller * more typescript * more typescript * fix the code to match the expected interface * very cool comment * interface correction * remove unused class method * add unit test for PdfMaker * file rename for typo correction Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

tsullivan added 8 commits October 20, 2020 13:38

convert pdf.js to TS

911c66a

more typescript

e9e2edb

simplify caller

2e0a624

more typescript

ca79e98

more typescript

bf3dec4

fix the code to match the expected interface

8125e3b

very cool comment

ef22b39

interface correction

b3743d3

tsullivan commented Oct 20, 2020

View reviewed changes

tsullivan changed the title ~~convert pdf.js to TS~~ [Reporting/Tech Debt] Convert PdfMaker class to TypeScript Oct 20, 2020

tsullivan added 2 commits October 20, 2020 20:25

remove unused class method

edf6af4

add unit test for PdfMaker

57b94a4

tsullivan commented Oct 21, 2020

View reviewed changes

tsullivan requested review from joelgriffith, poffdeluxe and a team October 21, 2020 14:13

tsullivan added v8.0.0 v7.11.0 (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes labels Oct 21, 2020

tsullivan marked this pull request as ready for review October 21, 2020 16:22

kibanamachine and others added 2 commits October 21, 2020 12:22

Merge branch 'master' into reporting/canvas-layout

3f2c6ab

file rename for typo correction

58dd494

poffdeluxe approved these changes Oct 21, 2020

View reviewed changes

Merge branch 'master' into reporting/canvas-layout

f4f6bbe

tsullivan merged commit ae95bf2 into elastic:master Oct 21, 2020

tsullivan deleted the reporting/canvas-layout branch October 21, 2020 20:54

tsullivan mentioned this pull request Oct 21, 2020

[7.x] [Reporting/Tech Debt] Convert PdfMaker class to TypeScript (#81242) #81400

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reporting/Tech Debt] Convert PdfMaker class to TypeScript #81242

[Reporting/Tech Debt] Convert PdfMaker class to TypeScript #81242

tsullivan commented Oct 20, 2020 •

edited

Loading

tsullivan Oct 20, 2020

tsullivan Oct 21, 2020

tsullivan Oct 21, 2020

tsullivan Oct 21, 2020 •

edited

Loading

tsullivan Oct 21, 2020

tsullivan commented Oct 21, 2020

poffdeluxe left a comment

kibanamachine commented Oct 21, 2020

	marginTop: headingMarginTop,
	marginBottom: headingMarginBottom,

[Reporting/Tech Debt] Convert PdfMaker class to TypeScript #81242

[Reporting/Tech Debt] Convert PdfMaker class to TypeScript #81242

Conversation

tsullivan commented Oct 20, 2020 • edited Loading

Summary

Checklist

For maintainers

tsullivan Oct 20, 2020

Choose a reason for hiding this comment

tsullivan Oct 21, 2020

Choose a reason for hiding this comment

tsullivan Oct 21, 2020

Choose a reason for hiding this comment

tsullivan Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

tsullivan Oct 21, 2020

Choose a reason for hiding this comment

tsullivan commented Oct 21, 2020

poffdeluxe left a comment

Choose a reason for hiding this comment

kibanamachine commented Oct 21, 2020

💚 Build Succeeded

Metrics [docs]

async chunks size

distributable file count

page load bundle size

History

tsullivan commented Oct 20, 2020 •

edited

Loading

tsullivan Oct 21, 2020 •

edited

Loading