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

Update styled_components_files.js to include all files that import styled-components #205011

Merged

Conversation

tkajtoch
Copy link
Member

@tkajtoch tkajtoch commented Dec 19, 2024

Summary

This PR updates the list of component files still using styled-components to match all files that import from styled-components package. This fixes the recent issue with some styled-components styles not loading properly in Kibana.

The previously used regex list became error-prone and is more complicated to follow and update when converting plugins to Emotion in steps. The added generator traverses kibana sources and looks up files that import styled-components to build an array of path regexes to put in packages/kbn-babel-preset/styled_components_files.js.

This code is an initial implementation to fix style errors on main and get us unblocked. It's missing tests and has not-so-optimal structure, however, the latter doesn't matter that much at the moment considering it's going to be run occasionally and styled-components is going away.

node scripts/styled_components_mapping

Local testing

Please run node scripts/build_kibana_platform_plugins --no-cache before testing this PR locally. Updates to @kbn/babel-preset don't update the optimizer cache key.

@tkajtoch tkajtoch added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v9.0.0 labels Dec 19, 2024
@tkajtoch tkajtoch self-assigned this Dec 19, 2024
@tkajtoch tkajtoch requested a review from a team as a code owner December 19, 2024 21:31
@dominiqueclarke dominiqueclarke self-requested a review December 19, 2024 21:49
@tkajtoch tkajtoch force-pushed the build/fix-babel-styled-components-files-list branch from 6141043 to 8cb350c Compare December 19, 2024 22:47
Copy link
Contributor

@Ikuni17 Ikuni17 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I think we will want to run the script against 8.x or possibly backport:prev-minor

@tkajtoch tkajtoch added the ci:cloud-deploy Create or update a Cloud deployment label Dec 20, 2024
@tkajtoch tkajtoch force-pushed the build/fix-babel-styled-components-files-list branch from 88524a0 to 20ae7f3 Compare December 20, 2024 04:02
Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

LGTM for Synthetics !!

image
image
image
image

@jasonrhodes jasonrhodes removed the request for review from dominiqueclarke December 20, 2024 15:35
@jasonrhodes
Copy link
Member

I removed @dominiqueclarke from the review requests here because @shahzad31 has looked at it and reported back in detail re: Synthetics. Thanks again, @tkajtoch!

@fkanout fkanout self-requested a review December 23, 2024 13:42
Copy link
Contributor

@PhilippeOberti PhilippeOberti left a comment

Choose a reason for hiding this comment

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

LGTM and desk tested for the Threat Hunting Investigations team. We will be trying to remove all these styled-components we own soon!

@clintandrewhall clintandrewhall force-pushed the build/fix-babel-styled-components-files-list branch from 89c808e to 17649a5 Compare December 24, 2024 18:21
@clintandrewhall
Copy link
Contributor

Resolved the rebase issues and added a couple of fixes.

@elastic/kibana-operations @mistic where is the appropriate place to run this script to ensure it's up-to-date? In yarn kbn boostrap or as part of the CI run, (e.g. "this file changed")...? If you can point us to the right place, I can either add that to this PR, or open a follow up.

@tkajtoch tkajtoch force-pushed the build/fix-babel-styled-components-files-list branch from 8b14214 to 8baaa61 Compare December 26, 2024 10:43
@fkanout
Copy link
Contributor

fkanout commented Dec 26, 2024

LGTM, alerting exp

@tkajtoch tkajtoch force-pushed the build/fix-babel-styled-components-files-list branch from 8cb5691 to 7fdf77d Compare January 4, 2025 02:24
@elasticmachine
Copy link
Contributor

elasticmachine commented Jan 4, 2025

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
integrationAssistant 470 469 -1
observabilityShared 238 237 -1
synthetics 977 976 -1
total -3

Async chunks

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

id before after diff
customIntegrations 81.0KB 83.9KB +2.9KB
exploratoryView 179.2KB 178.1KB -1.1KB
fleet 1.7MB 1.7MB -17.4KB
lists 146.1KB 146.1KB -2.0B
observability 478.6KB 473.2KB -5.4KB
observabilityShared 63.5KB 62.0KB -1.5KB
securitySolution 22.2MB 22.1MB -159.1KB
synthetics 927.5KB 913.8KB -13.7KB
timelines 30.5KB 30.3KB -203.0B
uptime 462.6KB 458.4KB -4.3KB
ux 165.4KB 164.7KB -734.0B
total -200.4KB

Page load bundle

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

id before after diff
exploratoryView 43.9KB 43.9KB -15.0B
fleet 171.2KB 170.9KB -271.0B
kibanaReact 36.9KB 36.9KB +10.0B
lists 3.7KB 3.7KB -14.0B
observability 102.1KB 101.9KB -170.0B
observabilityShared 93.9KB 93.3KB -587.0B
securitySolution 88.2KB 88.2KB -23.0B
synthetics 37.6KB 37.6KB +4.0B
timelines 201.7KB 201.6KB -96.0B
uptime 22.7KB 22.7KB -8.0B
total -1.1KB
Unknown metric groups

async chunk count

id before after diff
securitySolution 107 108 +1

ESLint disabled in files

id before after diff
@kbn/securitysolution-data-table 2 0 -2

ESLint disabled line counts

id before after diff
@kbn/react-kibana-context-styled 2 0 -2
@kbn/securitysolution-data-table 5 3 -2
customIntegrations 11 3 -8
triggersActionsUi 127 126 -1
total -13

Total ESLint disabled count

id before after diff
@kbn/react-kibana-context-styled 2 0 -2
@kbn/securitysolution-data-table 7 3 -4
customIntegrations 11 3 -8
triggersActionsUi 132 131 -1
total -15

History

cc @tkajtoch

@tkajtoch tkajtoch merged commit 211d4a6 into elastic:main Jan 5, 2025
10 checks passed
kowalczyk-krzysztof pushed a commit to kowalczyk-krzysztof/kibana that referenced this pull request Jan 7, 2025
PhilippeOberti added a commit that referenced this pull request Jan 8, 2025
…otion/css vs emotion/react (#205664)

## Summary

This recent [PR](#205011) slightly
broke the UI in a couple of small places in the alert details flyout.
Strangely, I did review the PR by pulling down the branch, but only
looked at the places that were impacted by the files modified. The
couple of places where the UI broke were completely different...
My guess it is is related to the fact that in those place we were still
using `@emotion/css` and this might not play nice with some
`styled_components`...

Updating those places to use `@emotion/react` fixed the issues!

| Before fix  | After fix |
| ------------- | ------------- |
|
![broken-1](https://github.com/user-attachments/assets/839760db-da3c-4031-b4be-18645b37c089)
|
![fix-1](https://github.com/user-attachments/assets/cdfae85c-0e63-45be-94dd-5e0f9a698d8a)
|

| Before fix  | After fix |
| ------------- | ------------- |
|
![broken-2](https://github.com/user-attachments/assets/22588529-5afd-491d-ab00-6e07593fb6f7)
|
![fix-2](https://github.com/user-attachments/assets/c078d814-1a33-49dc-aa0d-25dcff555de2)
|

| Before fix  | After fix |
| ------------- | ------------- |
|
![broken-3](https://github.com/user-attachments/assets/082d306c-8866-4e4f-ab18-db7c649101fe)
|
![fix-3](https://github.com/user-attachments/assets/5da76c44-934b-4a2a-a98e-2de34973d02e)
|

In a follow work, we need to remove completely all the
`styled_components` we have.
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 8, 2025
…otion/css vs emotion/react (elastic#205664)

## Summary

This recent [PR](elastic#205011) slightly
broke the UI in a couple of small places in the alert details flyout.
Strangely, I did review the PR by pulling down the branch, but only
looked at the places that were impacted by the files modified. The
couple of places where the UI broke were completely different...
My guess it is is related to the fact that in those place we were still
using `@emotion/css` and this might not play nice with some
`styled_components`...

Updating those places to use `@emotion/react` fixed the issues!

| Before fix  | After fix |
| ------------- | ------------- |
|
![broken-1](https://github.com/user-attachments/assets/839760db-da3c-4031-b4be-18645b37c089)
|
![fix-1](https://github.com/user-attachments/assets/cdfae85c-0e63-45be-94dd-5e0f9a698d8a)
|

| Before fix  | After fix |
| ------------- | ------------- |
|
![broken-2](https://github.com/user-attachments/assets/22588529-5afd-491d-ab00-6e07593fb6f7)
|
![fix-2](https://github.com/user-attachments/assets/c078d814-1a33-49dc-aa0d-25dcff555de2)
|

| Before fix  | After fix |
| ------------- | ------------- |
|
![broken-3](https://github.com/user-attachments/assets/082d306c-8866-4e4f-ab18-db7c649101fe)
|
![fix-3](https://github.com/user-attachments/assets/5da76c44-934b-4a2a-a98e-2de34973d02e)
|

In a follow work, we need to remove completely all the
`styled_components` we have.

(cherry picked from commit a8e1bf4)
kibanamachine added a commit that referenced this pull request Jan 8, 2025
… to emotion/css vs emotion/react (#205664) (#205928)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Security Solution][Alert Details] - fix some UI issues related to
emotion/css vs emotion/react
(#205664)](#205664)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Philippe
Oberti","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-01-08T16:00:28Z","message":"[Security
Solution][Alert Details] - fix some UI issues related to emotion/css vs
emotion/react (#205664)\n\n## Summary\r\n\r\nThis recent
[PR](#205011) slightly\r\nbroke
the UI in a couple of small places in the alert details
flyout.\r\nStrangely, I did review the PR by pulling down the branch,
but only\r\nlooked at the places that were impacted by the files
modified. The\r\ncouple of places where the UI broke were completely
different...\r\nMy guess it is is related to the fact that in those
place we were still\r\nusing `@emotion/css` and this might not play nice
with some\r\n`styled_components`...\r\n\r\nUpdating those places to use
`@emotion/react` fixed the issues!\r\n\r\n| Before fix | After fix
|\r\n| ------------- | -------------
|\r\n|\r\n![broken-1](https://github.com/user-attachments/assets/839760db-da3c-4031-b4be-18645b37c089)\r\n|\r\n![fix-1](https://github.com/user-attachments/assets/cdfae85c-0e63-45be-94dd-5e0f9a698d8a)\r\n|\r\n\r\n|
Before fix | After fix |\r\n| ------------- | -------------
|\r\n|\r\n![broken-2](https://github.com/user-attachments/assets/22588529-5afd-491d-ab00-6e07593fb6f7)\r\n|\r\n![fix-2](https://github.com/user-attachments/assets/c078d814-1a33-49dc-aa0d-25dcff555de2)\r\n|\r\n\r\n|
Before fix | After fix |\r\n| ------------- | -------------
|\r\n|\r\n![broken-3](https://github.com/user-attachments/assets/082d306c-8866-4e4f-ab18-db7c649101fe)\r\n|\r\n![fix-3](https://github.com/user-attachments/assets/5da76c44-934b-4a2a-a98e-2de34973d02e)\r\n|\r\n\r\nIn
a follow work, we need to remove completely all
the\r\n`styled_components` we
have.","sha":"a8e1bf46a3dc111e37caf19b19897081ed2a8078","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Threat
Hunting:Investigations","backport:version","v8.18.0"],"title":"[Security
Solution][Alert Details] - fix some UI issues related to emotion/css vs
emotion/react","number":205664,"url":"https://github.com/elastic/kibana/pull/205664","mergeCommit":{"message":"[Security
Solution][Alert Details] - fix some UI issues related to emotion/css vs
emotion/react (#205664)\n\n## Summary\r\n\r\nThis recent
[PR](#205011) slightly\r\nbroke
the UI in a couple of small places in the alert details
flyout.\r\nStrangely, I did review the PR by pulling down the branch,
but only\r\nlooked at the places that were impacted by the files
modified. The\r\ncouple of places where the UI broke were completely
different...\r\nMy guess it is is related to the fact that in those
place we were still\r\nusing `@emotion/css` and this might not play nice
with some\r\n`styled_components`...\r\n\r\nUpdating those places to use
`@emotion/react` fixed the issues!\r\n\r\n| Before fix | After fix
|\r\n| ------------- | -------------
|\r\n|\r\n![broken-1](https://github.com/user-attachments/assets/839760db-da3c-4031-b4be-18645b37c089)\r\n|\r\n![fix-1](https://github.com/user-attachments/assets/cdfae85c-0e63-45be-94dd-5e0f9a698d8a)\r\n|\r\n\r\n|
Before fix | After fix |\r\n| ------------- | -------------
|\r\n|\r\n![broken-2](https://github.com/user-attachments/assets/22588529-5afd-491d-ab00-6e07593fb6f7)\r\n|\r\n![fix-2](https://github.com/user-attachments/assets/c078d814-1a33-49dc-aa0d-25dcff555de2)\r\n|\r\n\r\n|
Before fix | After fix |\r\n| ------------- | -------------
|\r\n|\r\n![broken-3](https://github.com/user-attachments/assets/082d306c-8866-4e4f-ab18-db7c649101fe)\r\n|\r\n![fix-3](https://github.com/user-attachments/assets/5da76c44-934b-4a2a-a98e-2de34973d02e)\r\n|\r\n\r\nIn
a follow work, we need to remove completely all
the\r\n`styled_components` we
have.","sha":"a8e1bf46a3dc111e37caf19b19897081ed2a8078"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/205664","number":205664,"mergeCommit":{"message":"[Security
Solution][Alert Details] - fix some UI issues related to emotion/css vs
emotion/react (#205664)\n\n## Summary\r\n\r\nThis recent
[PR](#205011) slightly\r\nbroke
the UI in a couple of small places in the alert details
flyout.\r\nStrangely, I did review the PR by pulling down the branch,
but only\r\nlooked at the places that were impacted by the files
modified. The\r\ncouple of places where the UI broke were completely
different...\r\nMy guess it is is related to the fact that in those
place we were still\r\nusing `@emotion/css` and this might not play nice
with some\r\n`styled_components`...\r\n\r\nUpdating those places to use
`@emotion/react` fixed the issues!\r\n\r\n| Before fix | After fix
|\r\n| ------------- | -------------
|\r\n|\r\n![broken-1](https://github.com/user-attachments/assets/839760db-da3c-4031-b4be-18645b37c089)\r\n|\r\n![fix-1](https://github.com/user-attachments/assets/cdfae85c-0e63-45be-94dd-5e0f9a698d8a)\r\n|\r\n\r\n|
Before fix | After fix |\r\n| ------------- | -------------
|\r\n|\r\n![broken-2](https://github.com/user-attachments/assets/22588529-5afd-491d-ab00-6e07593fb6f7)\r\n|\r\n![fix-2](https://github.com/user-attachments/assets/c078d814-1a33-49dc-aa0d-25dcff555de2)\r\n|\r\n\r\n|
Before fix | After fix |\r\n| ------------- | -------------
|\r\n|\r\n![broken-3](https://github.com/user-attachments/assets/082d306c-8866-4e4f-ab18-db7c649101fe)\r\n|\r\n![fix-3](https://github.com/user-attachments/assets/5da76c44-934b-4a2a-a98e-2de34973d02e)\r\n|\r\n\r\nIn
a follow work, we need to remove completely all
the\r\n`styled_components` we
have.","sha":"a8e1bf46a3dc111e37caf19b19897081ed2a8078"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Philippe Oberti <[email protected]>
tkajtoch added a commit to tkajtoch/kibana that referenced this pull request Jan 10, 2025
tkajtoch added a commit that referenced this pull request Jan 10, 2025
…mport `styled-components` (#206084)

## Summary

This is a manual backport of
#205011 with regenerated
`styled_components_files.js` to match `8.x` code.

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Jan 13, 2025
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Jan 13, 2025
…otion/css vs emotion/react (elastic#205664)

## Summary

This recent [PR](elastic#205011) slightly
broke the UI in a couple of small places in the alert details flyout.
Strangely, I did review the PR by pulling down the branch, but only
looked at the places that were impacted by the files modified. The
couple of places where the UI broke were completely different...
My guess it is is related to the fact that in those place we were still
using `@emotion/css` and this might not play nice with some
`styled_components`...

Updating those places to use `@emotion/react` fixed the issues!

| Before fix  | After fix |
| ------------- | ------------- |
|
![broken-1](https://github.com/user-attachments/assets/839760db-da3c-4031-b4be-18645b37c089)
|
![fix-1](https://github.com/user-attachments/assets/cdfae85c-0e63-45be-94dd-5e0f9a698d8a)
|

| Before fix  | After fix |
| ------------- | ------------- |
|
![broken-2](https://github.com/user-attachments/assets/22588529-5afd-491d-ab00-6e07593fb6f7)
|
![fix-2](https://github.com/user-attachments/assets/c078d814-1a33-49dc-aa0d-25dcff555de2)
|

| Before fix  | After fix |
| ------------- | ------------- |
|
![broken-3](https://github.com/user-attachments/assets/082d306c-8866-4e4f-ab18-db7c649101fe)
|
![fix-3](https://github.com/user-attachments/assets/5da76c44-934b-4a2a-a98e-2de34973d02e)
|

In a follow work, we need to remove completely all the
`styled_components` we have.
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 ci:cloud-deploy Create or update a Cloud deployment release_note:skip Skip the PR/issue when compiling release notes v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.