Skip to content

cleaning up expression service types#80643

Merged
ppisljar merged 2 commits intoelastic:masterfrom
ppisljar:expressions/sessions
Oct 26, 2020
Merged

cleaning up expression service types#80643
ppisljar merged 2 commits intoelastic:masterfrom
ppisljar:expressions/sessions

Conversation

@ppisljar
Copy link
Copy Markdown
Contributor

@ppisljar ppisljar commented Oct 15, 2020

Summary

Cleans up some types on expressions and:

  • removes getInitialInput() from context (which was not used by anyone)
  • hides context.search behind context.getSearchContext() method
  • adds getSearchSessionId() to context as well as searchSessionId parameter to executor/loader

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@ppisljar ppisljar requested a review from a team as a code owner October 15, 2020 12:21
@botelastic botelastic bot added the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label Oct 15, 2020
@ppisljar ppisljar added backport:skip This PR does not require backporting Team:AppArch v7.11.0 v8.0.0 WIP Work in progress labels Oct 15, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@lukeelmers
Copy link
Copy Markdown
Contributor

I think Wylie just merged his PR which exposes debug mode in a slightly different way, hence the conflicts here

@ppisljar ppisljar force-pushed the expressions/sessions branch from 0754cd7 to c81a42e Compare October 20, 2020 12:06
@ppisljar ppisljar added release_note:skip Skip the PR/issue when compiling release notes and removed backport:skip This PR does not require backporting labels Oct 20, 2020
@ppisljar ppisljar force-pushed the expressions/sessions branch from c81a42e to 90b1f5b Compare October 20, 2020 14:05
@ppisljar ppisljar force-pushed the expressions/sessions branch from 90b1f5b to 613f873 Compare October 21, 2020 06:44
@ppisljar ppisljar requested a review from lukeelmers October 21, 2020 10:20
@ppisljar ppisljar removed the WIP Work in progress label Oct 21, 2020
@ppisljar ppisljar force-pushed the expressions/sessions branch from 613f873 to de93ecf Compare October 21, 2020 10:35
@ppisljar ppisljar requested a review from a team October 21, 2020 10:35
@ppisljar ppisljar force-pushed the expressions/sessions branch from de93ecf to 05aca66 Compare October 21, 2020 14:02
Copy link
Copy Markdown
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

From searchSessionid perspective lgtm.
Started using it in #81297 and works great.

Copy link
Copy Markdown
Contributor

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Couple minor TS nits, but overall this cleanup makes things much clearer! 👍

: createDefaultInspectorAdapters()) as InspectorAdapters,
inspectorAdapters:
execution.params.inspectorAdapters ||
((createDefaultInspectorAdapters() as any) as InspectorAdapters),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks like DefaultInspectorAdapters extends Adapters, why is the as any) as InspectorAdapters necessary here?

Comment on lines +53 to +55
query: [...toArray((getSearchContext() as any).query), ...toArray((input || {}).query)],
filters: [...((getSearchContext() as any).filters || []), ...((input || {}).filters || [])],
timeRange: (getSearchContext() as any).timeRange || (input ? input.timeRange : undefined),
Copy link
Copy Markdown
Contributor

@lukeelmers lukeelmers Oct 22, 2020

Choose a reason for hiding this comment

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

Feels like we shouldn't need to cast to any here -- presumably getSearchContext() is getting its type from ExecutionContextSearch, in which case query, filters, timeRange are all undefined so you should still be able to use them right?

(Or worst-case do getSearchContext().query!)

@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Metrics [docs]

page load bundle size

id before after diff
expressions 201.7KB 202.0KB +245.0B

History

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

Copy link
Copy Markdown
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Code review only of the kibana app team. LGTM!

@ppisljar ppisljar merged commit 48adb07 into elastic:master Oct 26, 2020
ppisljar added a commit to ppisljar/kibana that referenced this pull request Oct 26, 2020
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 26, 2020
…arm-phase-to-formlib

* 'master' of github.com:elastic/kibana:
  [Trigger Actions UI] Properly unmount app (elastic#81436)
  skip flaky suite (elastic#81576)
  skip flaky suite (elastic#78373)
  [Security Solution] Fix styling of EditDataProvider content (elastic#81456)
  [Search] Error Alignment 2 (elastic#80965)
  [APM] Unskip test (elastic#81574)
  [ML] Fix partition value selection on the Single Metric Viewer (elastic#81585)
  cleaning up expression service types (elastic#80643)
  Fix suggestions dropdown for query input (elastic#80990)
  [Usage collection] Make `schema` mandatory (elastic#79999)
  [ILM] Update show/hide data tier logic on cloud (elastic#81455)
  added brace import to advanced settings (elastic#81458)
  chore(NA): add spec-to-console and plugin-helpers as devOnly dependencies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants