-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Runtime field editor] Add server side for preview functionality #100029
[Runtime field editor] Add server side for preview functionality #100029
Conversation
d85f68d
to
4d680bc
Compare
deecf30
to
f394149
Compare
Pinging @elastic/kibana-app-services (Team:AppServices) |
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
@elastic/kibana-operations I had to update the plugin size limit as we have a tech debt on the field editor and need to change how we expose the field format editors to other plugins so make it lazy loaded instead of eagerly @mattkime . Do we know how many plugins currently use the public interface of the index pattern field editor to access the field format editors? |
@elasticmachine merge upstream |
…time-field-editor/preview-endpoint-2
@elasticmachine merge upstream |
…time-field-editor/preview-endpoint-2
@mattkime You've exposed the |
@elasticmachine merge upstream |
…time-field-editor/preview-endpoint-2
I haven't taken a look through the code but I know that its important to expose it for third party plugins. Custom field formatters are one of the most common reasons to create a third party plugin. |
@elasticmachine merge upstream |
…time-field-editor/preview-endpoint-2
}); | ||
}; | ||
|
||
return handleEsError({ error, response: res, handleCustomError }); |
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.
Nit: I think in our other plugins we've tended to inject the handleEsError
dependency, so line 31 would be something like this:
export const registerFieldPreviewRoute = ({ router, lib: { handleEsError } }: RouteDependencies): void => {
Not a blocker but it will make the code a little more consistent and would make it easier to mock this dependency if we ever write unit tests for these route handlers.
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.
Yes I remember that's the pattern I introduced in our plugins. I am not planning in adding unit tests for API route handlers as I tend to prefer integration tests for them.
I might update it though in future PR for the sake of alignement. 👍
We should probably look at breaking out field format editors into its own plugin. |
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 runs well, looks good!
Thanks for the review @mattkime and @cjcenizal ! |
ec64428
into
elastic:runtime-field-editor/preview-field-workstream
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
This PR contains the core logic of the preview functionality of runtime fields inside the index pattern field editor.
It does not contain any UI yet so I output for now the result in the browser console.
How to test
emit("bar")
)Notes on implementations
Added
<FieldEditorProvider />
To avoid property drilling I've added a
fieldEditorContext
to pass down services and context value to components down the tree. In this PR I haven't modified the existing code to reduce the amount of changes. In a following PR I will refactor to code to make use of the new context.New top level
__jest__
folderIn order to follow the pattern put in place in all of our apps I have created a top level
__jest__
folder to add the component integration tests. Now that we have server side endpoint to test, the helpers will help mock the HTTP request response easily and test different scenarios in the UI.In a following PR I will refactor the current component integration tests and move them to this folder.