-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Canvas] Extract and inject references for by-value embeddables #115124
[Canvas] Extract and inject references for by-value embeddables #115124
Conversation
2030a15
to
afea023
Compare
e4ae94d
to
cfd82be
Compare
afea023
to
6b2bc48
Compare
cfd82be
to
fc41b40
Compare
320e58c
to
98a4faf
Compare
6b2bc48
to
196b94d
Compare
98a4faf
to
bd12f1f
Compare
7557017
to
f8dbcaf
Compare
bd12f1f
to
2e57111
Compare
f8dbcaf
to
34831c5
Compare
2e57111
to
c9dfc60
Compare
⏳ Build in-progress, with failures
Failed CI Steps
History
To update your PR or re-run it, just comment with: |
💔 Build Failed
Failed CI StepsTest FailuresKibana Pipeline / jest / Jest Tests.x-pack/plugins/canvas/storybook.Storyshots components/WorkpadHeader/EditorMenu defaultStandard Out
Stack Trace
Kibana Pipeline / jest / Jest Tests.x-pack/plugins/canvas/storybook.Storyshots components/WorkpadHeader/EditorMenu dark modeStandard Out
Stack Trace
Kibana Pipeline / general / X-Pack API Integration Tests.x-pack/test/api_integration/apis/security_solution/tls·ts.apis SecuritySolution Endpoints Tls Test with Packetbeat Tls Overview Test Ensure data is returned for FlowTarget.DestinationStandard Out
Stack Trace
History
To update your PR or re-run it, just comment with: |
34831c5
to
6a29232
Compare
3e204cb
to
2371a43
Compare
6a29232
to
8ac6d41
Compare
2371a43
to
ed91534
Compare
Pinging @elastic/kibana-presentation (Team:Presentation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review only - Inject and Extract functions are looking pretty good so far!
Left a couple comments / questions, but much of it might just be me misunderstanding how reference extraction / injection works in Canvas.
x-pack/plugins/canvas/canvas_plugin_src/functions/external/embeddable.ts
Outdated
Show resolved
Hide resolved
|
||
// extracts references for by-reference embeddables | ||
if (input.savedObjectId) { | ||
const refName = 'embeddable.savedObjectId'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These references must get de-duped somewhere in the workpad extract / inject references, but I'm not sure where this happens. Just to make sure: If you look at the workpad in the saved objects explorer, each by reference embeddable has a unique ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: Looks like the de-duping is conducted properly inside the workpad, each reference is prepended with the element ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for double checking this for me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the one comment addressed, this LGTM!
c637735
to
780f71c
Compare
3fd223f
to
aeea737
Compare
Don't see any core/telemetry owned changes, removing us from reviews. |
state.type[0] = savedObjectReference.type; | ||
} else { | ||
// injects references for by-value embeddables | ||
const { type, ...injectedInput } = embeddablePersistableStateService.inject( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cqliu1 I think this is the source of the issue I mentioned to you. inject
expects embeddableStateWithType
and this is just giving it state.
const { type, ...injectedInput } = embeddablePersistableStateService.inject( | ||
input, | ||
references | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const { type, ...injectedInput } = embeddablePersistableStateService.inject( | |
input, | |
references | |
); | |
const { type, ...injectedInput } = embeddablePersistableStateService.inject( | |
{...input, type: state.type[0]}, | |
references | |
); |
1834014
to
151ceac
Compare
…tion Fixed server interpreter setup Register external functions in canvas_plugin_src plugin def
0104adf
to
7125b53
Compare
💛 Build succeeded, but was flaky
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now 👍
* [Canvas] Generic embeddable function (#104499) * Created generic embeddable function Fixed telemetry Updates expression on input change Fixed ts errors Store embeddable input to expression Added lib functions Added comments Fixed type errors Fixed ts errors Clean up Removed extraneous import Added context type to embeddable function def Fix import Update encode/decode fns Moved embeddable data url lib file Added embeddable test Updated comment * Fix reference extract/inject in embeddable fn * Simplify embeddable toExpression * Moved labsService to flyout.tsx * Added comment * [Canvas] Adds Save and Return Workflow (#111411) * [Canvas] Adds editor menu to Canvas (#113194) * Merge existing embeddable input with incoming embeddable input (#116026) * [Canvas] Extract and inject references for by-value embeddables (#115124) * Extract/inject references for by-value embeddables in embeddable function Fixed server interpreter setup Register external functions in canvas_plugin_src plugin def * Fixed ref name in embeddable.inject * Fixed ts errors * Fix missing type error Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Summary
Blocked by #113194.
This uses the Embeddable Persistable State Service to extract/inject references for by-value embeddables in the
embeddable
function.Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers