[Enterprise Search] Upgrade Kea to 2.3, update LogicMounter helper w/ props support #94232
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cee-chen
added
Feature:Plugins
release_note:skip
cee-chen
commented
Mar 9, 2021
| @@ -110,8 +111,10 @@ export class LogicMounter { | |||
| }; | |||
|
|
|||
| // Automatically reset context & mount the logic file | |||
| public mount = (values?: object) => { | |||
| public mount = (values?: object, props?: object) => { | |||
There was a problem hiding this comment.
Just want to quickly note - I'm not a super huge fan of how logic files that don't want defaults and only want props (e.g., AppLogic) now have to either pass undefined or {} as a first param. If I could go back in time I'd probably change this to
public mount = ({ defaults, props }) => {and have test files pass in their API like mount({ defaults: { foo: 'bar' } }) or mount({ props: { bar: 'baz' } }). IDK if we feel strongly to make that change in a ton of files at this point so I'm fine leaving it as-is, mostly just thinking out loud
There was a problem hiding this comment.
Also for more context, here's the first conversation where I mentioned LogicMounter needing to support props: #92560 (comment)
There was a problem hiding this comment.
I supposed you could do a mountWithProps, which would not require the empty object, but I am not a huge fan of that.
There was a problem hiding this comment.
Ya for sure, to clarify I don't hate this either and I'm fine with shipping it for now - we can definitely come back to this API later if we start using a ton more logic files w/ props!
scottybollinger
approved these changes
Mar 9, 2021
| this.resetContext(values); | ||
| if (props) this.logicFile.build(props); |
There was a problem hiding this comment.
I got that from Kea's docs here for context: https://kea.js.org/docs/guide/advanced#mounting-and-unmounting
// build the logic with props (`logic(props)` is short for `logic.build(props)`)
| @@ -110,8 +111,10 @@ export class LogicMounter { | |||
| }; | |||
|
|
|||
| // Automatically reset context & mount the logic file | |||
| public mount = (values?: object) => { | |||
| public mount = (values?: object, props?: object) => { | |||
There was a problem hiding this comment.
I supposed you could do a mountWithProps, which would not require the empty object, but I am not a huge fan of that.
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack Alerting API Integration Tests.x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/rbac_legacy·ts.alerting api integration security and spaces enabled Alerts legacy alerts alerts space_1_all_alerts_none_actions at space1 should schedule actions on legacy alertsStandard OutStack TraceMetrics [docs]Async chunks
To update your PR or re-run it, just comment with: |
|
Thanks for the reviews all! 🎉 I'm merging now to unblock myself from opening more PRs, but don't hesitate to leave more comments / make change requests @JasonStoltz - I'm happy to open up a follow up PR |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Mar 10, 2021
… props support (elastic#94232) * Enhance our LogicMounter helper to accept and build props * Update logic tests in AS that require props to now use LogicMounter * Upgrade Kea to 2.3.0 for future BindLogic use
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
The general theme of this PR is setting up Kea to work even better with
props. I'll shortly be using this for my upcoming CurationLogic work/PR, as I'll be passing acurationIdprop into CurationLogic.Kea upgrade (a71f142)
The Kea 2.3 upgrade will allow us to use a new BindLogic API: https://kea.js.org/blog/kea-2.3/
Which means I can avoid doing
useActions(CurationLogic({ curationId: 'cur-123456789' }))within<Curation />and just douseActions(CurationLogic).LogicMounter changes (82ae123 & 8a566ec)
This updates our
mounttest helper to allow passing in props as well as defaults. API looks the same as before, just with an extra param:Checklist
2.3 should not have introduced any breaking changes, but I did a basic QA smoke test of our App Search pages and confirmed they work as before.
Unit or functional tests were updated or added to match the most common scenarios