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.

Sign up for GitHub

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

Jump to bottom

[regression] ReactDOM.render calls need Render context in deprecated component #163137

Closed
wants to merge 1 commit into from
Closed

[regression] ReactDOM.render calls need Render context in deprecated component #163137

wants to merge 1 commit into from

Conversation

clintandrewhall
Copy link
Contributor

@clintandrewhall clintandrewhall commented Aug 4, 2023 •
edited
Loading

This is an alternative to #163103, and includes a fix @cee-chen and I discussed.

Summary

There's currently a regression when using the deprecated KibanaThemeProvider from kibana_react and kibana_utils.

Unfortunately, #161914 regressed #162365 in that many plugins and their Emotion styles (including EUI emotion styles) are now missing a cache and are being appended to to the end of the document <head> as opposed to within <meta name="emotion">.

What appears to be happening is many plugins are using a parent <KibanaThemeProvider> but not a parent <KibanaRootContextProvider> (not sure if this work is TBD or in progress). This means that a parent <EuiProvider>, (which determines the cache insertion of child Emotion styled components) is missing, which is causing several CSS specificity bugs, e.g. around datagrid.

The design for context use moving forward would be that we would use the Root context once, the Render context for out-of-band renders, and the Theme provider when adjusting the theme.

The mistake I made was deprecating the existing KibanaThemeProvider and mapping it directly to the new Theme context. My plan was to follow up and update all of the ReactDOM.render calls to use the Render provider, and believed my deprecation to be passive. It was, but not in all cases.

Until we can update all of our ReactDOM.render calls to use the correct context, we need to have the deprecated provider use the Render context. This will return us to the status quo until applications can make the change.

This PR also fixes an oversight where the Render provider still needs to utilize the EuiProvider, just not with the global styles. @cee-chen and I had several conversations clarifying which contexts were needed in each case, and it wasn't until we saw this issue that we realized we hadn't clarified enough. :-)

The good news is that @cee-chen and EUI are going to be working on their documentation and provider APIs to make the distinction easier. Even better news is, with these contexts, we can abstract any updates away from Kibana code. So all-in-all, we're still in a better position than when we started.

Next steps

  • Update all calls to ReactDOM.render to use the KibanaRenderContextProvider.
  • Remove all superfluous contexts from those calls.

QA

Previous/broken behavior

  • In your browser devtools inspector, open the document <head>
  • Under <meta name="emotion">, you'll only see a couple hundred style nodes:

  • If you scroll to the bottom of <head>, you should see a lot of <style data-emotion="css"> nodes:

Fixed behavior in this PR

  • In your browser devtools inspector, open the document <head>
  • Under <meta name="emotion">, you should now see well over thousands of styles

  • You should no longer see many <style data-emotion="css"> with EUI styles at the end of the head (there's a lot of <script> tags instead). There's still a few, but they appear to not be EUI styles (e.g. it's a plugin's random Emotion styles that they're using without a cache set up).

@clintandrewhall clintandrewhall added review regression loe:small Small Level of Effort release_note:skip Skip the PR/issue when compiling release notes impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. EUI Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) ci:build-storybooks v8.10.0 labels Aug 4, 2023
@clintandrewhall clintandrewhall requested review from a team as code owners August 4, 2023 02:01
@elasticmachine
Copy link
Contributor

Pinging @elastic/eui-team (EUI)

@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@clintandrewhall clintandrewhall mentioned this pull request Aug 4, 2023
Merged
clintandrewhall
clintandrewhall commented Aug 4, 2023
View reviewed changes
packages/react/kibana_context/root/root_provider.tsx
/** The `I18nStart` API from `CoreStart`. */
i18n: I18nStart;
i18n?: I18nStart;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cee-chen I'm not convinced this is the right way to handle the deprecation. We might consider your detect-warn-and-resolve approach.

@clintandrewhall clintandrewhall mentioned this pull request Aug 4, 2023
Merged
@kibana-ci
Copy link
Collaborator

kibana-ci commented Aug 4, 2023 •
edited
Loading

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #15 / FlyoutService openFlyout() with a currently active flyout replaces the current flyout with a new one
  • [job] [logs] Jest Tests #15 / ModalService openConfirm() renders a mountpoint confirm message
  • [job] [logs] Jest Tests #15 / ModalService openConfirm() renders a string confirm message
  • [job] [logs] Jest Tests #15 / ModalService openConfirm() with a currently active confirm replaces the current confirm with the new one
  • [job] [logs] Jest Tests #15 / ModalService openConfirm() with a currently active modal replaces the current modal with the new confirm
  • [job] [logs] Jest Tests #15 / ModalService openModal() with a currently active confirm replaces the current confirm with the new one
  • [job] [logs] Jest Tests #15 / ModalService openModal() with a currently active modal replaces the current modal with a new one
  • [job] [logs] Jest Tests #5 / renderUxApp has an error boundary for the UXAppRoot

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
kibanaReact 316 322 +6
kibanaUtils 172 177 +5
total +11

Async chunks

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

id before after diff
kibanaReact 209.3KB 209.3KB +3.0B
kibanaUtils 60.8KB 60.8KB +1.0B
total +4.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
core 369.9KB 370.0KB +64.0B
kibanaReact 51.3KB 52.2KB +907.0B
kibanaUtils 70.7KB 71.5KB +772.0B
total +1.7KB
Unknown metric groups

API count

id before after diff
@kbn/react-kibana-context-render 4 3 -1

History

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

@clintandrewhall
Copy link
Contributor Author

Closing in favor of merging some changes to the alternative detection method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cee-chen cee-chen Awaiting requested review from cee-chen

Assignees
No one assigned
Labels
ci:build-storybooks EUI impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:small Small Level of Effort regression release_note:skip Skip the PR/issue when compiling release notes review Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.10.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@clintandrewhall @elasticmachine @kibana-ci