[Embeddables Rebuild] Migrate Visualize #183197
Draft
+1,133
−1,706
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
…e-react-converstion
…e-react-converstion
…e-react-converstion # Conflicts: # src/plugins/visualizations/public/plugin.ts
Zacqary
added
Team:Presentation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
/ci |
…ub.com/Zacqary/kibana into visualize-embeddable-react-converstion
Zacqary
commented
May 17, 2024
| }, | ||
| references: [], | ||
| }} | ||
| parentApi={this._parentApi} |
There was a problem hiding this comment.
This pattern really bugs me. unifiedSearch properties are expected to be passed in through a parentApi, and they're supposed to be in the form of PublishingSubjects. But in this case (and with DefaultEditor) the current published values of each property is just a React prop. It makes sense from a React point of view to pass these in as props.
We can't, though, because the framework is expecting Observables. So instead we have to kind of break the React paradigm in order to shoehorn these props into some RxJS logic that they don't really want to be in.
Is there any better way to handle this?
There was a problem hiding this comment.
Is there any better way to handle this?
No. We want to avoid having multiple ways of doing things.
You could opt to create a component that takes timeRange, filters, and query as props and then encapsulates converting these to observables and passing via the parent API. Maps embeddable does this with the MapRenderer if you want to use this as an example.
There was a problem hiding this comment.
I'd say this frustration is warranted.
@nreese's suggestion may help - additionally you could use usePublishingSubject to handle the translation from Reactive variable -> PublishingSubject for you. I understand that neither of these solutions remove the translation requirement, they just make it a little easier.
I think the fundamental problem here is a mixup in the source of truth. Ideally if you were building an editor for a React Embeddable from scratch all state would be stored with RXJS. That said, it seems like changing the visualize architecture to be fully RXJS based is too large a lift for this PR, so you might be stuck translating for a while.
Ultimately, this is making me think that we shouldn't recommend folks use Embeddables in their standalone editors.
There was a problem hiding this comment.
We could also create a hook that could turn filters, timeRange, and query into a search API to turn this into a single line of code for consumers.
const searchApi = useSearchApi(filters, timeRange, query);
There was a problem hiding this comment.
That's not a bad idea. Kinda the opposite of useFetchContext
There was a problem hiding this comment.
right I can add it to this PR #183772
Ultimately, this is making me think that we shouldn't recommend folks use Embeddables in their standalone editors.
I think this highlights the embeddable key principles and best practices to only use embeddables when you need a component that can render in dashboard. For all other cases, expose a react Component. The problems that embeddables are trying to solve make them more difficult to work with then standard react components.
There was a problem hiding this comment.
I have added useSearchApi hook in #183772. You can check it out at https://github.com/elastic/kibana/blob/95428bd83f835ae8edefe833159fd2c60bd6da4a/packages/presentation/presentation_publishing/interfaces/fetch/publishes_unified_search.ts
Once that PR merges, you can just use that hook and this should save you a few lines and make working with embeddables more managable.
|
/ci |
…e-react-converstion # Conflicts: # packages/presentation/presentation_containers/index.ts
…ub.com/Zacqary/kibana into visualize-embeddable-react-converstion
|
/ci |
|
/ci |
…e-react-converstion # Conflicts: # src/plugins/visualizations/public/embeddable/visualize_embeddable.tsx
|
/ci |
|
/ci |
💔 Build FailedFailed CI Steps
Test Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
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
Closes #174958
This migrates the Visualize embeddable to the new React Embeddable framework.
Migrated:
Also adds deprecation statements to legacy Embeddable factories
Checklist