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.

Sign up for GitHub

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

Jump to bottom

[Expressions] Fix setup and start contracts #110841

Merged
merged 6 commits into from
Sep 23, 2021
Merged

[Expressions] Fix setup and start contracts #110841

merged 6 commits into from
Sep 23, 2021

Conversation

dokmic
Copy link
Contributor

@dokmic dokmic commented Sep 1, 2021 •
edited by ppisljar
Loading

Summary

Resolves #106509. The registerFunction, registerType, and registerRenderer functions cannot assert the right life-cycle state at the moment due to #106510 and #106511.

Checklist

For maintainers

@dokmic dokmic added review Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) v8.0.0 Team:AppServices release_note:skip Skip the PR/issue when compiling release notes v7.16.0 labels Sep 1, 2021
@dokmic dokmic marked this pull request as ready for review September 1, 2021 20:26
@dokmic dokmic requested review from a team as code owners September 1, 2021 20:26
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

ppisljar
ppisljar reviewed Sep 3, 2021
View reviewed changes
x-pack/plugins/canvas/server/saved_objects/workpad_references.ts Show resolved Hide resolved
x-pack/plugins/canvas/public/services/kibana/expressions.ts Show resolved Hide resolved
src/plugins/expressions/server/mocks.ts
getRenderer: jest.fn(),
getRenderers: jest.fn(),
getType: jest.fn(),
getTypes: jest.fn(),
Copy link
Member

Choose a reason for hiding this comment

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

we should most likely remove all the getters from setup contract as well as there is no way to be certain specific thing is going to be registered at the time you are requesting it (we can only guarantee that in start contract)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to remove that, but I found a few places where logic relies on those getters. I think it makes sense to keep them because it can be the case when you register functions conditionally.

Copy link
Member

Choose a reason for hiding this comment

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

can we remove those that are not used, seems only getFunction is used at the moment ?
and please open an issue for getFunction one.

  • the setup_expression.ts is not working reliably (possibly missing functions registered by other plugins)
  • the functions.ts should be using start contract
  • application.tsx should be using start contract.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are more methods, but they all are being called from the same place:

  • getFunction;
  • getFunctions;
  • getTypes.

I have created #112596 to remove that. Meantime, I've updated contracts and removed the ones that are not used. The rest I marked as deprecated to prevent new usages.

src/plugins/expressions/public/mocks.tsx Outdated Show resolved Hide resolved
ppisljar
ppisljar approved these changes Sep 21, 2021
View reviewed changes
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

@dokmic dokmic mentioned this pull request Sep 22, 2021
Closed
crob611
crob611 approved these changes Sep 22, 2021
View reviewed changes
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.

Changes in Canvas look good 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
expressions 1598 1626 +28

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
canvas 1.0MB 1.0MB -16.0B

Page load bundle

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

id before after diff
canvas 16.9KB 16.8KB -9.0B
expressions 127.1KB 128.3KB +1.3KB
total +1.3KB
Unknown metric groups

API count

id before after diff
expressions 2036 2072 +36

References to deprecated APIs

id before after diff
canvas 46 64 +18

History

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

@dokmic dokmic merged commit c06d604 into elastic:master Sep 23, 2021
@dokmic dokmic deleted the feature/106509 branch September 23, 2021 06:44
@dokmic dokmic mentioned this pull request Sep 23, 2021
Merged
jloleysens added a commit to jloleysens/kibana that referenced this pull request Sep 23, 2021
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into reporting/poc…
8361185 
…-migrate-away-from-injected-css-js

* 'master' of github.com:elastic/kibana: (189 commits)
  fix permissions for cloud test (elastic#112568)
  Updates the VEGA docs for v8.0 (elastic#112781)
  Updates the TSVB docs for v8 (elastic#112778)
  [Expressions] Fix setup and start contracts (elastic#110841)
  [DOCS] Update remote cluster and security links (elastic#112874)
  test/functional/apps/management/_test_huge_fields.js (elastic#112878)
  Fix the other one... (elastic#112873)
  [data.search.aggs] Use fields instead of _source in top_hits agg (elastic#109531)
  [Search sessions] Don't show incomplete warning if search requests aren't in session (elastic#112364)
  [data.search] Do not send ignore_throttled when search:includeFrozen is disabled (elastic#112755)
  [Monitoring] Add KQL filter bar to alerts (elastic#111663)
  Log deprecation warnings for plugins which won't be disable-able in 8.0 (elastic#112602)
  [CI] Balance CI Groups (elastic#112836)
  Add ILM URLs to documentation link service (elastic#112748)
  Bump chromedriver to 93 (elastic#112847)
  [Maps] move joins from LayerDescriptor to VectorLayerDescriptor (elastic#112427)
  Add a handler for a possible promise rejection (elastic#112840)
  Removes space, fix build (elastic#112856)
  [Maps] fix unhandled promise rejections in jest tests (elastic#112712)
  Copy pass 3 (elastic#112815)
  ...

# Conflicts:
#	src/plugins/dashboard/public/application/dashboard_app.tsx
#	src/plugins/dashboard/public/application/embeddable/viewport/dashboard_viewport.tsx
dokmic added a commit that referenced this pull request Sep 23, 2021
@dokmic @kibanamachine
[Expressions] Fix setup and start contracts (#110841) (#112923)
7ce80d5 
* Refactor executor forking to implement state inheritance
* Fix setup and start contracts typings
* Add support of named forks
* Add expressions service life-cycle assertions

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
lykkin pushed a commit to lykkin/kibana that referenced this pull request Sep 28, 2021
@dokmic @lykkin
[Expressions] Fix setup and start contracts (elastic#110841)
be57ca9 
* Refactor executor forking to implement state inheritance
* Fix setup and start contracts typings
* Add support of named forks
* Add expressions service life-cycle assertions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stacey-gammon stacey-gammon stacey-gammon left review comments

@crob611 crob611 crob611 approved these changes

@ppisljar ppisljar ppisljar approved these changes

Assignees
No one assigned
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:skip Skip the PR/issue when compiling release notes review v7.16.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Expressions] expressions setup/start contract leak wrong functions
6 participants
@dokmic @elasticmachine @kibanamachine @crob611 @ppisljar @stacey-gammon