TypeScript Reporting Layouts by bgaddis56 · Pull Request #22454 · elastic/kibana

bgaddis56 · 2018-08-28T13:45:28Z

Change the layout code to TypeScript format as well as create a new set of layout classes.

elasticmachine · 2018-08-28T13:45:30Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

WIP

Made sure all types are set on the new layout classes.

stacey-gammon · 2018-08-28T13:48:48Z

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

Preservelayout => PreserveLayout (capital L)

stacey-gammon · 2018-08-28T13:51:18Z

x-pack/plugins/reporting/export_types/printable_pdf/server/lib/layouts/layout.ts

I think this can be private

stacey-gammon · 2018-08-28T13:59:30Z

src/server/index.d.ts

kbn_server => KbnServer

stacey-gammon · 2018-08-28T14:02:20Z

src/server/index.d.ts

+  }
+
+export interface configObject {
+    get: (path: string) => ConfigEntry;


I think ConfigEntry should be any. I know we generally avoid any as much as possible, but here, you really can't type what will be returned unless you know what path is. I think there is a way to map what the string is to expected object, but for the sake of this PR, I say just make any here for now, and move the below typings to the reporting folder, so you can still do get captureConfig(): ConfigEntry {

stacey-gammon · 2018-08-28T14:03:09Z

x-pack/plugins/reporting/export_types/printable_pdf/server/lib/generate_pdf.js

 import { oncePerServer } from '../../../../server/lib/once_per_server';
 import { screenshotsObservableFactory } from './screenshots';
-import { getLayoutFactory } from './layouts';
+import { getlayout } from './layouts/layout_factory';


getlayout => getLayout. actually, I'd name is createLayout since it's creating a new object, not getting an existing one. (Though I know the current code uses getLayout so if that ends up requiring a large refactor, fine to leave as is)

stacey-gammon · 2018-08-28T14:21:30Z

x-pack/plugins/reporting/export_types/printable_pdf/server/lib/layouts/preserve_layout.ts

+}
+
+export class Preservelayout extends Layout {
+  public groupCount: number = 1;


This is never modified so can be a const too.

stacey-gammon · 2018-08-28T14:21:46Z

x-pack/plugins/reporting/export_types/printable_pdf/server/lib/layouts/preserve_layout.ts

+} from '../../../../../../../../src/server/index';
+import { Layout } from './layout';
+
+interface Pagesizeparams {


Pagesizeparams => PageSizeParams

stacey-gammon · 2018-08-28T14:35:17Z

x-pack/plugins/reporting/export_types/printable_pdf/server/lib/layouts/optimized_layout.ts

+    return this.server.config();
+  }
+
+  get captureConfig(): ConfigEntry {


I'm not a fan of the get syntax, and I think it adds unnecessary execution cycles. Can just make captureConfig a property on the class (i think const since it never changes), same with most of the other ones. e.g.:

private const captureConfig: CaptureConfig; constructor(server: kbn_server, id: string) { super(id); //(getting rid of server object into base class re: other comment) this.captureConfig = server.config().get('xpack.reporting.capture') }

actually... if the server object is only used to grab that capture config, you might not need to keep a reference around to the server object at all. This is more in line with the original code, which also did not keep a reference to the server object in the returned object.

Actually, can just pass in captureConfig object to the constructor and do the grabbing of it in the factory function. Would make this class easier to test.

private const captureConfig: CaptureConfig; constructor(captureConfig: CaptureConfig, id: string) { super(id); //(getting rid of server object into base class re: other comment) this.captureConfig = captureConfig; }

stacey-gammon · 2018-08-28T14:42:10Z

x-pack/plugins/reporting/export_types/printable_pdf/server/lib/layouts/preserve_layout.ts

+  };
+
+  public height: number = 0;
+  public width: number = 0;


I think these can be readonly and private:

private readonly height: number; private readonly width: number;

stacey-gammon · 2018-08-28T14:43:16Z

x-pack/plugins/reporting/export_types/printable_pdf/server/lib/layouts/preserve_layout.ts

+    return this.height * this.zoom;
+  }
+
+  get scaledWidth(): number {


instead of get functions, just make a class property, this.scaledWidth;

Changed location of type interfaces and fixed naming errors

…cript-reporting-layouts Merge of latest changes

stacey-gammon

Getting close!!

stacey-gammon · 2018-08-28T19:42:31Z

src/server/index.d.ts

+
+export interface configObject {
+    get: (path: string) => any;
+}


hm, the aligning in this file didn't get auto corrected. Maybe because of the .d.ts extension.

can you update so it's only two space indentations? and the } on line 8 should have no spaces before it.

stacey-gammon · 2018-08-28T19:42:47Z

x-pack/plugins/reporting/export_types/printable_pdf/server/lib/generate_pdf.js

 import { oncePerServer } from '../../../../server/lib/once_per_server';
 import { screenshotsObservableFactory } from './screenshots';
-import { getLayoutFactory } from './layouts';
+import { createlayout } from './layouts/layout_factory';


createlayout => createLayout

stacey-gammon · 2018-08-28T19:43:49Z

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

+    zoom: number;
+    width: number;
+    height: number;
+  }


same comment with this file and the space aligning. Looks like we'll have to update .d.ts files automatically. This will probably get hit by the linter when ci runs.

stacey-gammon · 2018-08-28T19:44:31Z

x-pack/plugins/reporting/export_types/printable_pdf/server/lib/layouts/layout.ts

+ * you may not use this file except in compliance with the Elastic License.
+ */
+import { Size } from '../../../../../types';
+import { ViewZoomWidthHeight } from './index.d';


shouldn't need the .d in the name, and I don't think you need to specify index too. Try:

import { ViewZoomWidthHeight } from '.';

For the record, this doesn't work because it resolves it to index.js which exists in the same location, so have to be explicit with index.d.ts. Maybe the file should just be named types.d.ts in that case. Which also matches our other types.d.ts file.

stacey-gammon · 2018-08-28T19:46:18Z

x-pack/plugins/reporting/export_types/printable_pdf/server/lib/layouts/layout.ts

+
+  public abstract getPdfPageOrientation(): string | undefined;
+
+  public abstract getPdfPageSize(pageSizeParamsIn: PageSizeParams): string | Size;


I would just call is pageSizeParams and drop the In part of the name. Not a common convention in our code base to suffix input parameters with In.

stacey-gammon · 2018-08-28T19:50:20Z

x-pack/plugins/reporting/export_types/printable_pdf/server/lib/layouts/print_layout.ts

+import path from 'path';
+import { KbnServer } from '../../../../../../../../src/server/index';
+import { Size } from '../../../../../types';
+import { CaptureConfig } from './index.d';


import { CaptureConfig } from '.';

stacey-gammon · 2018-08-28T19:51:17Z

x-pack/plugins/reporting/export_types/printable_pdf/server/lib/layouts/print_layout.ts

+  evaluate: (evaluateOptions: EvaluateOptions) => void;
+}
+
+const groupCount: number = 2;


Think this will cause a bug - need to move groupCount into the class, same as PreserveLayout so it's accessible from outside. Make public readonly

stacey-gammon · 2018-08-28T19:52:42Z

x-pack/plugins/reporting/export_types/printable_pdf/server/lib/layouts/print_layout.ts

+type EvalArgs = any[];
+
+interface EvaluateOptions {
+  // Fn is a function in string form to avoid tslint from auto formatting it into a version not


Fn => fn since it's referencing a variable, probably shouldn't be capitalized. Maybe even add back tics around it.

stacey-gammon · 2018-08-28T19:53:19Z

x-pack/plugins/reporting/types.d.ts

+    debug: (message: string) => void;
+    error: (message: string) => void;
+    warning: (message: string) => void;
+  }


alignment in this file, like others.

stacey-gammon · 2018-08-28T19:53:45Z

x-pack/plugins/reporting/export_types/printable_pdf/server/lib/layouts/layout_factory.ts

+// you'll notice that we aren't passing the zoom at this time, while it'd be possible to use
+// window.pixelDensity to figure out what the current user is seeing, if they're going to send the
+// PDF to someone else, I can see there being benefit to using a higher pixel density, so we're
+// going to leave this hard-coded for the time being


Still need to remove this

Fix naming on properties and methods as well as a few other fixes

stacey-gammon · 2018-08-28T21:35:46Z

src/server/index.d.ts

+  config: () => configObject;
+}
+
+export interface configObject {


configObject => ConfigObject

stacey-gammon · 2018-08-28T21:36:22Z

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

-    viewport: Size;
+  zoom: number;
+  viewport: Size;
  }


-2 spaces for the }

stacey-gammon · 2018-08-28T21:37:14Z

jenkins, test this

Name Changes and spacing

elasticmachine · 2018-08-28T21:51:36Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

stacey-gammon · 2018-08-28T21:55:15Z

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

+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+export { createlayout } from './layout_factory';


createlayout => createLayout

Name Change

stacey-gammon · 2018-08-28T22:19:14Z

jenkins, test this

elasticmachine · 2018-08-28T22:44:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

stacey-gammon · 2018-08-28T22:54:19Z

Error is because of the import path for KbnServer. Will try to find out tomorrow what the path should be.

Changes for typescript import and direct reference to layout_factory

stacey-gammon · 2018-08-29T14:06:22Z

jenkins, test this

elasticmachine · 2018-08-29T14:32:31Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…cript-reporting-layouts

stacey-gammon · 2018-08-30T17:30:36Z

jenkins, test this

elasticmachine · 2018-08-30T19:01:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

stacey-gammon

Woot! Just sign the CLA and it's an LGTM from me!

stacey-gammon · 2018-08-30T20:29:06Z

x-pack/plugins/reporting/export_types/printable_pdf/server/lib/layouts/print_layout.ts

+interface EvaluateOptions {
+  // 'fn' is a function in string form to avoid tslint from auto formatting it into a version not
+  // underfood by transform_fn safeWrap.
+  fn: ((...evalArgs: EvalArgs) => any) | string;


we can actually get rid of the | string part now, since we switched back to a function.

…sistency Removed String as a type and renamed index.d.ts to types.d.ts for consistency

stacey-gammon · 2018-08-30T21:51:57Z

jenkins, test this

tsullivan · 2018-08-30T22:14:39Z

x-pack/plugins/reporting/export_types/printable_pdf/server/lib/layouts/layout_factory.ts

+  dimensions: Size;
+}
+
+export function createLayout(server: KbnServer, layoutParams: LayoutParams): Layout {


let's call this file create_layout.ts to name it after what its primary export is.

tsullivan · 2018-08-30T22:38:18Z

x-pack/plugins/reporting/export_types/printable_pdf/server/lib/layouts/preserve_layout.ts

+import { Size } from '../../../../../types';
+import { Layout, PageSizeParams } from './layout';
+
+const ZOOM: number = 2;


Zoom used to be a param that could be passed in from a caller, but 2 was the default. This makes it completely hardcoded.

Also, you've removed the explanatory comments: https://github.com/elastic/kibana/blob/master/x-pack/plugins/reporting/export_types/printable_pdf/server/lib/layouts/preserve_layout.js#L9-L12

Intentional. This setting was never exposed to the user as configurable - #20091

I don't think we ever will expose it. The only reason we use it is to make sure the resolution is a little bigger on the screenshots.

From Brandon in that above issue:

The zoom is an undocumented setting that I intended to remove, but never got around to it.

tsullivan

Like to see the same mechanism for zoom able to be passed in for preserve_layout. Even if callers are not passing it and the constructor always uses its default. Also, keep in the comments that got removed.

elasticmachine · 2018-08-30T23:25:49Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

stacey-gammon · 2018-08-30T23:39:52Z

@tsullivan re:

Like to see the same mechanism for zoom able to be passed in for preserve_layout. Even if callers are not passing it and the constructor always uses its default. Also, keep in the comments that got removed.

I disagree that this should be passed in as a param. No other value is passed in and the setting is untested and undocumented. Happy to discuss if you disagree though!

stacey-gammon · 2018-08-31T00:30:14Z

Looks like the pie chart didn't render. Could be flaky, it's happened before.

jenkins, test this

elasticmachine · 2018-08-31T02:05:45Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

stacey-gammon · 2018-08-31T12:20:18Z

Sadly, it appears reporting tests are flaky again. Not the fault of this PR

stacey-gammon · 2018-08-31T12:50:59Z

jenkins, test this

elasticmachine · 2018-08-31T13:23:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

stacey-gammon · 2018-08-31T14:34:23Z

Ci is currently broken. Don't worry about the red build for now. Another fix is going in shortly to stabilize the reporting tests. Will kick this off when ci is back up!

stacey-gammon · 2018-09-03T12:48:52Z

retest

elasticmachine · 2018-09-03T14:20:23Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

stacey-gammon · 2018-09-04T14:21:15Z

Spoke to @tsullivan offline. We agreed it would have been preferred to do the zoom refactor in a separate PR from the typescript (my bad, I steered this change towards that), but it's fine to leave as is now that it's done.

Regarding the comment about the zoom, we agreed it would be nice to leave in the reason for why zoom is set to 2 (for higher resolution). I'll follow up with that in a separate PR so we can get this checked in, as @bgaddis56 will be offline for a few weeks and we don't want to hold this up for that long.

tl;dr; good to go! I will merge shortly.

tsullivan · 2018-09-04T16:03:42Z

Thanks for helping me understand the context, @stacey-gammon

LGTM!

* wip WIP * Changed any ypes to actual types Made sure all types are set on the new layout classes. * Changes recommended from code review Changed location of type interfaces and fixed naming errors * Latest Code Review Changes Fix naming on properties and methods as well as a few other fixes * Name Changes and spacing Name Changes and spacing * Name Change Name Change * Changes for typescript import and direct reference to layout_factory Changes for typescript import and direct reference to layout_factory * Move types locally * Evaluate function changes for puppeteer * Removed String as a type and renamed index.d.ts to types.d.ts for consistency Removed String as a type and renamed index.d.ts to types.d.ts for consistency * Changed layout_factoy to create_layout

stacey-gammon · 2018-09-04T16:35:45Z

Congrats on getting your first PR merged @bgaddis56! 🎉

* wip WIP * Changed any ypes to actual types Made sure all types are set on the new layout classes. * Changes recommended from code review Changed location of type interfaces and fixed naming errors * Latest Code Review Changes Fix naming on properties and methods as well as a few other fixes * Name Changes and spacing Name Changes and spacing * Name Change Name Change * Changes for typescript import and direct reference to layout_factory Changes for typescript import and direct reference to layout_factory * Move types locally * Evaluate function changes for puppeteer * Removed String as a type and renamed index.d.ts to types.d.ts for consistency Removed String as a type and renamed index.d.ts to types.d.ts for consistency * Changed layout_factoy to create_layout

stacey-gammon self-requested a review August 28, 2018 13:47

stacey-gammon added :Sharing zDeprecated Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead v7.0.0 v6.5.0 labels Aug 28, 2018

bgaddis56 added 2 commits August 28, 2018 09:57

wip

6e7c5fb

WIP

Changed any ypes to actual types

f2108a3

Made sure all types are set on the new layout classes.

bgaddis56 force-pushed the typescript-reporting-layouts branch from c08aca2 to f2108a3 Compare August 28, 2018 14:00

stacey-gammon reviewed Aug 28, 2018

View reviewed changes

bgaddis56 added 2 commits August 28, 2018 15:28

Changes recommended from code review

704d79f

Changed location of type interfaces and fixed naming errors

Merge branch 'master' of https://github.com/elastic/kibana into types…

b9e7482

…cript-reporting-layouts Merge of latest changes

stacey-gammon suggested changes Aug 28, 2018

View reviewed changes

Latest Code Review Changes

6d805c5

Fix naming on properties and methods as well as a few other fixes

stacey-gammon suggested changes Aug 28, 2018

View reviewed changes

Name Changes and spacing

9e83af3

Name Changes and spacing

stacey-gammon reviewed Aug 28, 2018

View reviewed changes

Name Change

8979f2e

Name Change

Changes for typescript import and direct reference to layout_factory

4d0d3d0

Changes for typescript import and direct reference to layout_factory

bgaddis56 added 3 commits August 30, 2018 11:07

Merge branch 'master' of https://github.com/elastic/kibana into types…

af4257a

…cript-reporting-layouts

Move types locally

4464fb2

Evaluate function changes for puppeteer

ca4b974

stacey-gammon approved these changes Aug 30, 2018

View reviewed changes

stacey-gammon reviewed Aug 30, 2018

View reviewed changes

Removed String as a type and renamed index.d.ts to types.d.ts for con…

a285cbf

…sistency Removed String as a type and renamed index.d.ts to types.d.ts for consistency

tsullivan reviewed Aug 30, 2018

View reviewed changes

tsullivan requested changes Aug 30, 2018

View reviewed changes

Changed layout_factoy to create_layout

ced32f9

tsullivan approved these changes Sep 4, 2018

View reviewed changes

stacey-gammon merged commit 1464741 into elastic:master Sep 4, 2018

stacey-gammon mentioned this pull request Sep 4, 2018

[6.x] TypeScript Reporting Layouts (#22454) #22669

Merged

bgaddis56 deleted the typescript-reporting-layouts branch September 24, 2018 13:31


		public abstract getPdfPageOrientation(): string \| undefined;

		public abstract getPdfPageSize(pageSizeParamsIn: PageSizeParams): string \| Size;

Conversation

bgaddis56 commented Aug 28, 2018

Uh oh!

elasticmachine commented Aug 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stacey-gammon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stacey-gammon commented Aug 28, 2018

Uh oh!

elasticmachine commented Aug 28, 2018

💔 Build Failed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stacey-gammon commented Aug 28, 2018

Uh oh!

elasticmachine commented Aug 28, 2018

💔 Build Failed

Uh oh!

stacey-gammon commented Aug 28, 2018

Uh oh!

stacey-gammon commented Aug 29, 2018

Uh oh!

elasticmachine commented Aug 29, 2018

💔 Build Failed

Uh oh!

stacey-gammon commented Aug 30, 2018

Uh oh!

elasticmachine commented Aug 30, 2018

💚 Build Succeeded

Uh oh!

stacey-gammon left a comment

Choose a reason for hiding this comment