[Reporting] Clean Up TypeScript Definitions#76566
Merged
tsullivan merged 19 commits intoelastic:masterfrom Sep 22, 2020
Merged
Conversation
6528dd2 to
0acf89a
Compare
0acf89a to
d90df7a
Compare
d90df7a to
ff9d552
Compare
tsullivan
commented
Sep 3, 2020
Member
Author
There was a problem hiding this comment.
headers comes from req - the encrypted header string was not used.
tsullivan
commented
Sep 3, 2020
Member
Author
There was a problem hiding this comment.
the get_csv_job module is only used for immediate csv download
d02b367 to
f4eaff9
Compare
tsullivan
commented
Sep 3, 2020
Member
Author
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
Member
Author
There was a problem hiding this comment.
Which is why applying jobSettings has moved to this file
tsullivan
commented
Sep 3, 2020
Member
Author
There was a problem hiding this comment.
"print" layout does not have the dimension fields but is still a LayoutInstance
c86351e to
49eaeef
Compare
tsullivan
commented
Sep 3, 2020
Member
Author
There was a problem hiding this comment.
removed this, as it was duplicate with ReportDocument
2287b01 to
2197898
Compare
tsullivan
commented
Sep 3, 2020
Member
Author
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
| _source: { | ||
| status: 'completed', | ||
| output: { max_size_reached: false, csv_contains_formulas: false }, | ||
| payload: { type: 'spectacular', title: 'specimen' }, |
Member
Author
There was a problem hiding this comment.
This is why we should not have had .type on JobSummary
tsullivan
commented
Sep 19, 2020
| id: src._id, | ||
| status: src._source.status, | ||
| title: src._source.payload.title, | ||
| type: src._source.payload.type, |
Member
Author
There was a problem hiding this comment.
There is no type field of payload
Contributor
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
Member
Author
|
@elasticmachine merge upstream |
Member
Author
|
@elasticmachine merge upstream |
streamich
approved these changes
Sep 22, 2020
Contributor
streamich
left a comment
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();
Contributor
💛 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>
Member
Author
|
@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
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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