[Reporting] Clean Up TypeScript Definitions #76566
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tsullivan
force-pushed
the
reporting/switch-to-tm-pre
branch
5 times, most recently
from
6528dd2 to
0acf89a
Compare
tsullivan
added
release_note:skip
tsullivan
force-pushed
the
reporting/switch-to-tm-pre
branch
from
0acf89a to
d90df7a
Compare
tsullivan
changed the title
tsullivan
force-pushed
the
reporting/switch-to-tm-pre
branch
from
d90df7a to
ff9d552
Compare
tsullivan
commented
Sep 3, 2020
| const logger = parentLogger.clone([CSV_FROM_SAVEDOBJECT_JOB_TYPE, 'create-job']); | ||
|
|
||
| return async function scheduleTask(jobParams, headers, context, req) { | ||
| return async function createJob(jobParams, context, req) { |
There was a problem hiding this comment.
headers comes from req - the encrypted header string was not used.
tsullivan
commented
Sep 3, 2020
| @@ -13,7 +13,7 @@ describe('Get CSV Job', () => { | |||
| let mockSavedObjectsClient: any; | |||
| let mockUiSettingsClient: any; | |||
| beforeEach(() => { | |||
| mockJobParams = { isImmediate: true, savedObjectType: 'search', savedObjectId: '234-ididid' }; | |||
There was a problem hiding this comment.
the get_csv_job module is only used for immediate csv download
tsullivan
force-pushed
the
reporting/switch-to-tm-pre
branch
2 times, most recently
from
d02b367 to
f4eaff9
Compare
tsullivan
commented
Sep 3, 2020
|
|
||
| // store the pending report, puts it in the Reporting Management UI table | ||
| const report = await store.addReport(exportType.jobType, user, payload); |
There was a problem hiding this comment.
the argument types of addReport changed to take a single report instead of 3 separate parts of a report
There was a problem hiding this comment.
Which is why applying jobSettings has moved to this file
tsullivan
commented
Sep 3, 2020
| @@ -63,4 +63,4 @@ interface LayoutSelectors { | |||
| positionElements?: (browser: HeadlessChromiumDriver, logger: LevelLogger) => Promise<void>; | |||
| } | |||
|
|
|||
| export type LayoutInstance = Layout & LayoutSelectors & Size; | |||
| export type LayoutInstance = Layout & LayoutSelectors & Partial<Size>; | |||
There was a problem hiding this comment.
"print" layout does not have the dimension fields but is still a LayoutInstance
tsullivan
force-pushed
the
reporting/switch-to-tm-pre
branch
5 times, most recently
from
c86351e to
49eaeef
Compare
tsullivan
commented
Sep 3, 2020
| type: string; | ||
| } | ||
|
|
||
| export interface JobSource<JobParamsType> { |
There was a problem hiding this comment.
removed this, as it was duplicate with ReportDocument
tsullivan
force-pushed
the
reporting/switch-to-tm-pre
branch
4 times, most recently
from
2287b01 to
2197898
Compare
tsullivan
commented
Sep 3, 2020
| size: number; | ||
| max_size_reached?: boolean; | ||
| warnings?: string[]; | ||
| } |
There was a problem hiding this comment.
I created a new directory for these, because more code is coming soon
tsullivan
commented
Sep 19, 2020
| @@ -23,30 +24,30 @@ const mockJobsFound = [ | |||
| _source: { | |||
| status: 'completed', | |||
| output: { max_size_reached: false, csv_contains_formulas: false }, | |||
| payload: { type: 'spectacular', title: 'specimen' }, | |||
There was a problem hiding this comment.
This is why we should not have had .type on JobSummary
tsullivan
commented
Sep 19, 2020
| return { | ||
| id: src._id, | ||
| status: src._source.status, | ||
| title: src._source.payload.title, | ||
| type: src._source.payload.type, |
There was a problem hiding this comment.
There is no type field of payload
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
streamich
approved these changes
Sep 22, 2020
There was a problem hiding this comment.
Changes LGTM, ran it on Mac/Chrome all seems to work.
While looking at this PR, I also looked at the reporting plugin in general to learn it better, bellow are few things that caught my attention.
Maybe we should retire the dashboard sharing API share.register and use there UI Actions instead, where dashboard would have its own trigger, say DASHBOARD_MENU_TRIGGER:
uiActions.addTriggerAction(DASHBOARD_MENU_TRIGGER, action);To reduce bundle sizes, ideally we want to load apps and management app section asynchronously, so would be nice to load the management app section using a dynamic import, and maybe even the UI parts of registered dashboard menu actions.
I liked the .stop$ observable, I'm thinking maybe we could even change how we write Kibana plugins, to something like this:
const {plugin, setup$, start$, stop$} = createPlugin();
export {plugin};
setup$
.pipe(
tap((core, plugins) => { /* ... */ }),
)
.subscribe();
start$
.pipe(
tap((core, plugins) => { /* ... */ }),
)
.subscribe();
💛 Build succeeded, but was flaky
Test FailuresChrome X-Pack UI Functional Tests.x-pack/test/functional/apps/lens/smokescreen·ts.lens app lens smokescreen tests should create an xy visualization with filters aggregationStandard OutStack TraceBuild metricspage load bundle size
distributable file count
History
To update your PR or re-run it, just comment with: |
tsullivan
added a commit
to tsullivan/kibana
that referenced
this pull request
Sep 22, 2020
* [Reporting] Simplify Export Type Definitions, use defaults for generics, refactor * ReportApiJSON interface for common * rename JobSummary to JobStatusBucket for clarity * revert unneeded create mock changes * clean up the diff * revert changes to worker.js * rewrite comment * rename type to jobtype in JobStatusBucket * allow type inference * JobSummarySet * remove odd comment * Reflect that browser timezone may be undefined in the BaseParams * comment about optional browserTimezone * revert unecessary es archive change Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
tsullivan
added a commit
that referenced
this pull request
Sep 23, 2020
* [Reporting] Clean Up TypeScript Definitions (#76566) * [Reporting] Simplify Export Type Definitions, use defaults for generics, refactor * ReportApiJSON interface for common * rename JobSummary to JobStatusBucket for clarity * revert unneeded create mock changes * clean up the diff * revert changes to worker.js * rewrite comment * rename type to jobtype in JobStatusBucket * allow type inference * JobSummarySet * remove odd comment * Reflect that browser timezone may be undefined in the BaseParams * comment about optional browserTimezone * revert unecessary es archive change Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> * fix ts Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
|
@streamich Thanks for taking the time to review this and offer those suggestions. I think each one of them should stay tracked, so I am filing separate ER issues.
I'm not sure I follow this one specifically. I think it is about a cleanup for App Arch in code that I'm not that familiar with. Feel free to ping me and we can talk about it more.
++ Reducing bundle sizes is hugely important for Reporting as a feature because lower bundle sizes means faster load in the headless browser that captures the screenshots. Of course we don't care about any management features in that use case, so if we can avoid having the browser parse the code altogether, that is great. Filed: #78242
This reminded me that we need to monitor Filed: #78243 |
This was referenced Sep 23, 2020
sophiec20
added
the
(Deprecated) Feature:Reporting
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Pull work out of the PR to switch Reporting to Task Manager
LayoutParamsand is converted toLayoutInstanceon the serverside, withcreateLayoutexport_types/commonJobSource/ (ReportDocument)basePathfrom job params, since it is looked up at execution timeChecklist
Delete any items that are not applicable to this PR.
For maintainers