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

cleaning up expression service types #80643

Merged
merged 2 commits into from
Oct 26, 2020

Conversation

ppisljar
Copy link
Member

@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 commit does not require backporting Team:AppArch v7.11.0 v8.0.0 WIP Work in progress labels Oct 15, 2020
@elasticmachine
Copy link
Contributor

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

@lukeelmers
Copy link
Member

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

@ppisljar ppisljar added release_note:skip Skip the PR/issue when compiling release notes and removed backport:skip This commit does not require backporting labels Oct 20, 2020
@ppisljar ppisljar removed the WIP Work in progress label Oct 21, 2020
@ppisljar ppisljar requested a review from a team October 21, 2020 10:35
Copy link
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
Member

@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
Member

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
Member

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
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
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.

None yet

6 participants