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.

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

[Lens] Remove <NativeRenderer /> #161521

Merged
merged 3 commits into from
Jul 11, 2023

Conversation

mbondyra
Copy link
Contributor

@mbondyra mbondyra commented Jul 10, 2023

Summary

The NativeRenderer component is currently used to mount another component in a separate mounting point. As far as I recall, we introduced to allow users to create visualizations in non-React frameworks. The idea was that users could write their own Lens visualizations or datasources code and integrate it with our system. However, it seems that this concept hasn't gained traction and we don’t have it prioritized. Even if users express interest in writing their visualizations outside of React, it is still possible to do so with some additional boilerplate code (which we could provide as an example non-React visualization).
Pros:

  1. Simplifies and shortens the code:
    1.1. Testing and debugging become easier as we no longer need to check separate React trees when integrating frame, data source, and visualization components.
    1.2. Components communicate using standard React patterns, making maintenance and comprehension simpler.
    1.3. Context providers no longer need to be passed to each separate component since they are already within the context.
    1.4. Easier propagation of events or any other form of inter-component communication.

  2. Greatly improves performance and facilitates maintenance:
    2.1. Directly accessing context inside the DatasourcePanel eliminates the need for context passing, resulting in better performance.
    2.2. Removing the requirement for a separate React root also contributes to improved performance.

  3. The render method will be removed when we upgrade to React 18. While we could replace it with the new createRoot method, it makes sense to perform some cleanup ;)

Cons:

  1. Setting up non-React visualization or data source code might become slightly more complex.

Performance improvement for drag and drop action with these changes:

before:

Screenshot 2023-07-10 at 07 14 39

after:

Screenshot 2023-07-10 at 07 16 24

Single render when dragging:

(the first image is 3 screenshots from 3 different react roots as they have separate mounting point. The complete render time is ~380ms)
Screenshot 2023-07-10 at 07 16 24

After we have one common render tree. Because we don't have to pass context down as a prop, we greatly reduced the number of components rerendered. (I will be working on reducing the render time for workspace panel as this seems to still be a bottleneck point)
Screenshot 2023-07-10 at 14 52 41

@mbondyra mbondyra force-pushed the lens/refactor_domreactrender branch from 5a2d815 to 9644a3e Compare July 10, 2023 07:31
@mbondyra mbondyra added Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v8.10.0 labels Jul 10, 2023
@mbondyra mbondyra marked this pull request as ready for review July 10, 2023 13:19
@mbondyra mbondyra requested review from a team as code owners July 10, 2023 13:19
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations @elastic/kibana-visualizations-external (Team:Visualizations)

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

kibana-gis changes LGTM
code review only

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Great cleanup, LGTM!

@mbondyra mbondyra enabled auto-merge (squash) July 11, 2023 07:41
@mbondyra mbondyra disabled auto-merge July 11, 2023 07:43
@mbondyra mbondyra force-pushed the lens/refactor_domreactrender branch from 2396d86 to 5d99a96 Compare July 11, 2023 10:04
@mbondyra mbondyra requested a review from a team as a code owner July 11, 2023 10:04
@mbondyra mbondyra force-pushed the lens/refactor_domreactrender branch from 2bf0d3a to 189b6e6 Compare July 11, 2023 11:07
@mbondyra mbondyra removed the request for review from a team July 11, 2023 11:07
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 1005 1003 -2

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
lens 535 519 -16

Async chunks

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

id before after diff
lens 1.3MB 1.3MB -5.9KB
maps 2.7MB 2.7MB -129.0B
total -6.1KB
Unknown metric groups

API count

id before after diff
lens 632 616 -16

ESLint disabled line counts

id before after diff
enterpriseSearch 14 16 +2
securitySolution 409 413 +4
total +6

References to deprecated APIs

id before after diff
lens 87 82 -5

Total ESLint disabled count

id before after diff
enterpriseSearch 15 17 +2
securitySolution 488 492 +4
total +6

History

  • 💚 Build #141475 succeeded 2396d86bd80738324b27f6fc5ea390670a793dc7
  • 💚 Build #141219 succeeded 4d65418be529f93a6e5569d13941837da1f4ce27
  • 💔 Build #141157 failed b7460e887257e09bea30ce67a191d6b823ef0206
  • 💔 Build #141145 failed 9644a3ed74bec93acfe1d0c5d74e65046b52bd22
  • 💔 Build #141137 failed 5a2d815c47c2008ac18bbbe86d6da4caa8d142f1

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

@mbondyra mbondyra merged commit 95e5087 into elastic:main Jul 11, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 11, 2023
@mbondyra mbondyra deleted the lens/refactor_domreactrender branch July 11, 2023 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants