Skip to content
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

[Canvas] Cleanup types in lib #94517

Merged
merged 10 commits into from
Mar 16, 2021
Merged

Conversation

mshustov
Copy link
Contributor

Summary

Extracted from #92175
Fixes TS errors in Canvas code that start failing when tests import generated d.ts files.

@mshustov mshustov added chore Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.13.0 labels Mar 12, 2021
@mshustov mshustov requested a review from a team as a code owner March 12, 2021 09:14
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@mshustov mshustov changed the title Canvas ts cleanup [Canvas] Cleanup types in lib Mar 12, 2021
image: resolveWithMissingImage(args.image, elasticOutline),
emptyImage: resolveWithMissingImage(args.emptyImage),
image: resolveWithMissingImage(args.image, elasticOutline) as string,
emptyImage: resolveWithMissingImage(args.emptyImage) as string,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably resolveWithMissingImage should return strings only instead of string | null? I left the current behavior since it's to be desirable according to the unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, that should be cleaned up.

@mshustov mshustov enabled auto-merge (squash) March 12, 2021 12:12
@mshustov mshustov added the auto-backport Deprecated - use backport:version if exact versions are needed label Mar 12, 2021
Copy link
Contributor

@crob611 crob611 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 👍

@@ -52,7 +50,7 @@ export interface CanvasRenderable {
state: 'ready' | 'error';
value: {
as: string;
containerStyle: ContainerStyle;
containerStyle: any;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this ContainerStyle type not importable from somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment says // @ts-expect-error Unlinked Webpack Type. No idea what it means, but tsc uses any instead

image: resolveWithMissingImage(args.image, elasticOutline),
emptyImage: resolveWithMissingImage(args.emptyImage),
image: resolveWithMissingImage(args.image, elasticOutline) as string,
emptyImage: resolveWithMissingImage(args.emptyImage) as string,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, that should be cleaned up.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
canvas 548.8KB 549.4KB +553.0B

History

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

@mshustov mshustov merged commit 09f90d8 into elastic:master Mar 16, 2021
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 16, 2021
* fix get_legend_config error in canvas/lib/index

* convert resolve_dataurl to ts to fix canvas/lib/index failure

* convert expression_form_handler to ts to fix canvas/lib/index failure

* convert canvas lib/error into ts

* canvas: do not compile json file due to effect on performance

* remove type. it is not exported and inferred as any implicitly

* fix datatable error in lib/index.d.ts file

* fix url resolver

* case manually to avoid incompatibility error
@kibanamachine
Copy link
Contributor

💚 Backport successful

7.x / #94706

Successful backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Mar 16, 2021
* fix get_legend_config error in canvas/lib/index

* convert resolve_dataurl to ts to fix canvas/lib/index failure

* convert expression_form_handler to ts to fix canvas/lib/index failure

* convert canvas lib/error into ts

* canvas: do not compile json file due to effect on performance

* remove type. it is not exported and inferred as any implicitly

* fix datatable error in lib/index.d.ts file

* fix url resolver

* case manually to avoid incompatibility error

Co-authored-by: Mikhail Shustov <restrry@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed chore release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants