[Canvas] Convert Autocomplete to Typescript #42502

crob611 · 2019-08-02T02:24:10Z

Summary

The test for Autocomplete was still in mocha, and the new interpreter functions for embeddables was causing that to break. So this converts the Autocomplete library to typescript, and it's test to Jest.

This also cleans up the duplicate AST type definition that we had in our types with the existing ExpressionAST type in core.

Because this is moving some files from .js to .ts and then heavily modifying, git has a difficult time showing the diff as actual moves. I've split this up into two commits, the first of which is only the file moves, and the second are the actual changes. Viewing just the second commit should make the diff a lot cleaner.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~

Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-08-02T02:24:12Z

Pinging @elastic/kibana-canvas

elasticmachine · 2019-08-02T03:27:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

clintandrewhall

This is really impressive. The arg value ignore is the only piece preventing me from accepting this outright... feel free to hit me up if you want to talk through it.

clintandrewhall · 2019-08-12T15:23:53Z

x-pack/legacy/plugins/canvas/common/lib/autocomplete.ts

+}
+
+// Wraps ExpressionArg with meta or replace ExpressionAST with ExpressionASTWithMeta
+type WrapExpressionArgWithMeta<T> = T extends ExpressionAST


You should chat with @ppisljar and @lukeelmers about these types, and/or consider putting them in a shared file for expressions. They seem useful beyond autocomplete.

clintandrewhall · 2019-08-12T15:24:52Z

x-pack/legacy/plugins/canvas/common/lib/autocomplete.ts

+  ast: ExpressionASTWithMeta,
+  position: number
+): { ast: ExpressionASTWithMeta; fnIndex: number; argName?: string; argIndex?: number } {
+  const fnIndex = ast.node.chain.findIndex(fn => fn.start <= position && position <= fn.end);


This could probably use some comments... it's pretty dense.

clintandrewhall · 2019-08-12T15:27:23Z

x-pack/legacy/plugins/canvas/common/lib/autocomplete.ts

+      return true;
+    }
+    return !argEntries.some(([name, values]) => {
+      // @ts-ignore ArgValue doesn't have a required name type, but actual Args do


This ignore is repeated several times... why does the value not match the type specified on line 219? It sounds like ArgValue needs to have name be included and optional?

All of our function definitions are typed as ExpressionFunction. When they are added to the interpreters FunctionRegistry, they are run through this Fn method which slightly alters the type that it spits out, (one of which is that name is attached to the arg). The existing code was using that name property, which TS is complaining about.

Now that I'm looking at this code again, I think I can change it to work without using that name, since I also have the KEY that is the same as that name but we should probably fix either that Fn method or the ExpressionFunction type in case there are other dissimilarities that pop up.

clintandrewhall · 2019-08-12T15:28:55Z

x-pack/legacy/plugins/canvas/server/usage/workpad_collector.ts

@@ -87,7 +73,7 @@ export function summarizeWorkpads(workpadDocs: Workpad[]): WorkpadTelemetry {
    );
    const functionCounts = workpad.pages.reduce<number[]>((accum, page) => {
      return page.elements.map(element => {
-        const ast: AST = fromExpression(element.expression) as AST; // TODO: Remove once fromExpression is properly typed
+        const ast: ExpressionAST = fromExpression(element.expression) as ExpressionAST; // TODO: Remove once fromExpression is properly typed


@ppisljar @lukeelmers : will fromExpression be properly typed anytime soon?

@ppisljar Any reason not to go ahead and type it as this ExpressionAST? I can add that to this PR if needed?

@crob611 The ExpressionAST type should be safe to use here.

@clintandrewhall It looks like a type for fromExpression is already defined in the old interpreter package (packages/kbn-interpreter/src/common/lib/ast.d.ts), but it uses AST: unknown (this was before we added typings for the AST). This section must have been overlooked when we made that update; I'll make a note to fix it, and can remove the casting you've done here at that time.

(or @crob611 if you want to make that change in this PR feel free to do that as well!)

(sorry for all the comments)

The issue you are going to run into is that the interface for the AST is defined in the data plugin, which means you can't import it into the old interpreter package. So it may be better to remove the fromExpression type from the interpreter package, and move it to the data plugin to export from there.

@lukeelmers How would you set the type for something in kbn/interpreter from within the data plugin? Would you have to use like declare module 'kbn-interpreter?

elasticmachine · 2019-08-13T15:24:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-13T16:02:24Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

crob611 · 2019-08-13T16:11:11Z

retest

elasticmachine · 2019-08-13T17:27:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-13T20:00:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

clintandrewhall

I'm going to approve to unblock you, so we can get this in... be sure to address the types questions with @lukeelmers, either in this PR or a follow-up, if that makes more sense.

Great work here...!

elasticmachine · 2019-08-14T21:17:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Move the files around * Convert Auto Complete to TS. Convert Collectors to ExpressionAST type * Remove TS-Lint ignores * Add comment for getFnArgPosition method

…p-metrics-selectall * 'master' of github.com:elastic/kibana: (50 commits) [Uptime] update monitor list configs for mobile view (elastic#43218) [APM] Local UI filters (elastic#41588) [Code] Upgrade ctags langserver (elastic#43252) [Code] show multiple definition results in panel (elastic#43249) Adds Metric Type to full screen launch tracking (elastic#42692) [Canvas] Convert Autocomplete to Typescript (elastic#42502) [telemetry] add spacesEnabled config back to xpack_main (elastic#43312) [ML] Adds DF Transform Analytics list to Kibana management (elastic#43151) Add TLS client authentication support. (elastic#43090) [csp] Telemetry for csp configuration (elastic#43223) [SIEM] Run Cypress Tests Against Elastic Cloud & Cypress Command Line / Reporting (elastic#42804) docs: add tip on agent config in a dt (elastic#43301) [ML] Adding bucket span estimator to new wizards (elastic#43288) disable flaky tests (elastic#43017) Fix percy target branch for PRs (elastic#43160) [ML] Adding post create job options (elastic#43205) Restore discover histogram selection triggering fetch (elastic#43097) Per panel time range (elastic#43153) [Infra UI] Add APM to Metadata Endpoint (elastic#42197) Sentence case copy changes (elastic#43215) ...

* Move the files around * Convert Auto Complete to TS. Convert Collectors to ExpressionAST type * Remove TS-Lint ignores * Add comment for getFnArgPosition method

Corey Robertson added 2 commits August 1, 2019 22:00

Move the files around

a64206a

Convert Auto Complete to TS. Convert Collectors to ExpressionAST type

e7766bf

crob611 added review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:medium Medium Level of Effort v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.4.0 technical_debt labels Aug 2, 2019

crob611 requested a review from clintandrewhall August 2, 2019 02:24

crob611 requested a review from a team as a code owner August 2, 2019 02:24

crob611 mentioned this pull request Aug 2, 2019

[Canvas] Canvas embeddable #39839

Merged

7 tasks

clintandrewhall reviewed Aug 12, 2019

View reviewed changes

Corey Robertson added 3 commits August 13, 2019 10:11

Merge branch 'master' into autocomplete-convert

2269ada

Remove TS-Lint ignores

e1c1c1e

Add comment for getFnArgPosition method

c61041d

Merge branch 'master' into autocomplete-convert

60bee61

clintandrewhall approved these changes Aug 13, 2019

View reviewed changes

Merge branch 'master' into autocomplete-convert

29d59a7

crob611 merged commit 67cc190 into elastic:master Aug 14, 2019

crob611 mentioned this pull request Aug 14, 2019

[7.x] [Canvas] Convert Autocomplete to Typescript (#42502) #43328

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Canvas] Convert Autocomplete to Typescript #42502

[Canvas] Convert Autocomplete to Typescript #42502

crob611 commented Aug 2, 2019 •

edited

Loading

elasticmachine commented Aug 2, 2019

elasticmachine commented Aug 2, 2019

clintandrewhall left a comment

clintandrewhall Aug 12, 2019

clintandrewhall Aug 12, 2019

clintandrewhall Aug 12, 2019

crob611 Aug 12, 2019

clintandrewhall Aug 12, 2019

crob611 Aug 12, 2019

lukeelmers Aug 12, 2019

lukeelmers Aug 12, 2019

lukeelmers Aug 12, 2019

crob611 Aug 13, 2019

elasticmachine commented Aug 13, 2019

elasticmachine commented Aug 13, 2019

crob611 commented Aug 13, 2019

elasticmachine commented Aug 13, 2019

elasticmachine commented Aug 13, 2019

clintandrewhall left a comment

elasticmachine commented Aug 14, 2019

[Canvas] Convert Autocomplete to Typescript #42502

[Canvas] Convert Autocomplete to Typescript #42502

Conversation

crob611 commented Aug 2, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Aug 2, 2019

elasticmachine commented Aug 2, 2019

💚 Build Succeeded

clintandrewhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 13, 2019

💚 Build Succeeded

elasticmachine commented Aug 13, 2019

💔 Build Failed

crob611 commented Aug 13, 2019

elasticmachine commented Aug 13, 2019

💔 Build Failed

elasticmachine commented Aug 13, 2019

💚 Build Succeeded

clintandrewhall left a comment

Choose a reason for hiding this comment

elasticmachine commented Aug 14, 2019

💚 Build Succeeded

crob611 commented Aug 2, 2019 •

edited

Loading