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

chore: deprecate euiThemeVars #202943

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

weronikaolejniczak
Copy link

@weronikaolejniczak weronikaolejniczak commented Dec 4, 2024

Summary

As we transition from euiThemeVars (@kbn/ui-theme) to useEuiTheme (@elastic/eui), we plan to include a deprecation notice to discourage the use of the legacy JSON tokens.

Context: #199715 (comment)

closes #8201

@weronikaolejniczak weronikaolejniczak requested a review from a team as a code owner December 4, 2024 14:35
@weronikaolejniczak weronikaolejniczak added skip-ci backport:skip This commit does not require backporting labels Dec 4, 2024
@weronikaolejniczak
Copy link
Author

@elasticmachine merge upstream

@weronikaolejniczak weronikaolejniczak added the release_note:skip Skip the PR/issue when compiling release notes label Dec 4, 2024
@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 4, 2024

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #2 / apis index_patterns index_patterns/_fields_for_wildcard route fields_for_wildcard_route response returns 200 when index is closed
  • [job] [logs] FTR Configs #2 / apis index_patterns index_patterns/_fields_for_wildcard route fields_for_wildcard_route response returns 200 when index is closed
  • [job] [logs] Jest Integration Tests #5 / incompatible_cluster_routing_allocation retries the INIT action with a descriptive message when cluster settings are incompatible
  • [job] [logs] Jest Integration Tests #5 / incompatible_cluster_routing_allocation retries the INIT action with a descriptive message when cluster settings are incompatible
  • [job] [logs] Jest Integration Tests #5 / migration v2 - read batch size does not reduce the read batchSize in half if no batches exceeded maxReadBatchSizeBytes
  • [job] [logs] Jest Integration Tests #5 / migration v2 - read batch size does not reduce the read batchSize in half if no batches exceeded maxReadBatchSizeBytes
  • [job] [logs] Jest Integration Tests #5 / migration v2 - read batch size reduces the read batchSize in half if a batch exceeds maxReadBatchSizeBytes
  • [job] [logs] Jest Integration Tests #5 / migration v2 - read batch size reduces the read batchSize in half if a batch exceeds maxReadBatchSizeBytes
  • [job] [logs] Jest Integration Tests #5 / migration v2 migrates saved objects normally with multiple ES nodes
  • [job] [logs] Jest Integration Tests #5 / migration v2 migrates saved objects normally with multiple ES nodes
  • [job] [logs] FTR Configs #41 / security app users should show the default roles
  • [job] [logs] FTR Configs #41 / security app users should show the default roles
  • [job] [logs] FTR Configs #38 / serverless observability UI Dataset Quality Degraded fields flyout testing root cause for ignored fields current field limit issues should let user increase the field limit for integrations
  • [job] [logs] FTR Configs #76 / Upgrade Assistant Upgrade Assistant GET /api/upgrade_assistant/status returns a successful response
  • [job] [logs] FTR Configs #76 / Upgrade Assistant Upgrade Assistant GET /api/upgrade_assistant/status returns a successful response
  • [job] [logs] FTR Configs #95 / X-Pack Accessibility Tests - Group 1 Kibana users Accessibility a11y test for roles drop down
  • [job] [logs] FTR Configs #95 / X-Pack Accessibility Tests - Group 1 Kibana users Accessibility a11y test for roles drop down

Metrics [docs]

Unknown metric groups

References to deprecated APIs

id before after diff
@kbn/ai-assistant 0 15 +15
@kbn/alerts-ui-shared 8 11 +3
@kbn/cell-actions 0 5 +5
@kbn/chart-icons 0 3 +3
@kbn/cloud-security-posture 0 28 +28
@kbn/coloring 0 21 +21
@kbn/discover-contextual-components 0 4 +4
@kbn/discover-utils 0 4 +4
@kbn/ecs-data-quality-dashboard 0 27 +27
@kbn/elastic-assistant 0 56 +56
@kbn/grid-layout 0 20 +20
@kbn/grouping 0 18 +18
@kbn/ml-data-grid 0 2 +2
@kbn/ml-field-stats-flyout 0 3 +3
@kbn/monaco 10 46 +36
@kbn/react-kibana-context-styled 1 3 +2
@kbn/search-api-panels 0 2 +2
@kbn/search-index-documents 0 2 +2
@kbn/security-api-key-management 0 3 +3
@kbn/securitysolution-exception-list-components 0 38 +38
@kbn/shared-ux-file-upload 0 2 +2
@kbn/unified-data-table 0 7 +7
@kbn/unified-field-list 3 6 +3
@kbn/visualization-ui-components 0 27 +27
aiops 4 6 +2
alerting 83 90 +7
cases 31 33 +2
charts 0 2 +2
cloudSecurityPosture 6 30 +24
console 1 4 +3
controls 13 15 +2
customIntegrations 0 40 +40
datasetQuality 0 4 +4
dataVisualizer 11 27 +16
devTools 0 2 +2
discover 9 15 +6
enterpriseSearch 17 32 +15
eventAnnotationListing 2 24 +22
expressionLegacyMetricVis 0 2 +2
expressionMetricVis 0 5 +5
home 7 9 +2
indexLifecycleManagement 1 12 +11
indexManagement 0 13 +13
infra 6 14 +8
inspector 0 9 +9
interactiveSetup 0 7 +7
inventory 0 2 +2
kubernetesSecurity 1 6 +5
lens 84 143 +59
logsExplorer 0 2 +2
maps 33 49 +16
ml 34 95 +61
monitoring 2 4 +2
observability 0 2 +2
observabilityLogsExplorer 0 5 +5
presentationPanel 0 14 +14
savedObjects 62 64 +2
savedObjectsTagging 37 39 +2
searchInferenceEndpoints 0 5 +5
security 27 33 +6
securitySolution 450 546 +96
spaces 13 15 +2
triggersActionsUi 27 33 +6
unifiedDocViewer 0 2 +2
unifiedSearch 13 26 +13
visTypeTable 0 2 +2
visTypeVega 1 10 +9
total +848

History

@clintandrewhall
Copy link
Contributor

It looks like this whole package should be deprecated. @mistic @elastic/kibana-operations how do we typically deprecate an entire package in Kibana, (and evangelize that)? Is it an eslint rule?

@weronikaolejniczak
Copy link
Author

@clintandrewhall that makes sense! Looking at .eslintrc.js it seems that I can add it to the RESTRICTED_IMPORTS array:

kibana/.eslintrc.js

Lines 195 to 274 in e067fa2

/** Restricted imports with suggested alternatives */
const RESTRICTED_IMPORTS = [
{
name: 'lodash',
importNames: ['set', 'setWith', 'template'],
message:
'lodash.set/setWith: Please use @kbn/safer-lodash-set instead.\n' +
'lodash.template: Function is unsafe, and not compatible with our content security policy.',
},
{
name: 'lodash.set',
message: 'Please use @kbn/safer-lodash-set/set instead',
},
{
name: 'lodash.setwith',
message: 'Please use @kbn/safer-lodash-set/setWith instead',
},
{
name: 'lodash/set',
message: 'Please use @kbn/safer-lodash-set/set instead',
},
{
name: 'lodash/setWith',
message: 'Please use @kbn/safer-lodash-set/setWith instead',
},
{
name: 'lodash/fp',
importNames: ['set', 'setWith', 'assoc', 'assocPath', 'template'],
message:
'lodash.set/setWith/assoc/assocPath: Please use @kbn/safer-lodash-set/fp instead\n' +
'lodash.template: Function is unsafe, and not compatible with our content security policy.',
},
{
name: 'lodash/fp/set',
message: 'Please use @kbn/safer-lodash-set/fp/set instead',
},
{
name: 'lodash/fp/setWith',
message: 'Please use @kbn/safer-lodash-set/fp/setWith instead',
},
{
name: 'lodash/fp/assoc',
message: 'Please use @kbn/safer-lodash-set/fp/assoc instead',
},
{
name: 'lodash/fp/assocPath',
message: 'Please use @kbn/safer-lodash-set/fp/assocPath instead',
},
{
name: 'lodash.template',
message: 'lodash.template is unsafe, and not compatible with our content security policy.',
},
{
name: 'lodash/template',
message: 'lodash.template is unsafe, and not compatible with our content security policy.',
},
{
name: 'lodash/fp/template',
message: 'lodash.template is unsafe, and not compatible with our content security policy.',
},
{
name: 'react-use',
message: 'Please use react-use/lib/{method} instead.',
},
{
name: 'react-router-dom',
importNames: ['Router', 'Switch', 'Route'],
message: 'Please use @kbn/shared-ux-router instead',
},
{
name: '@kbn/kibana-react-plugin/public',
importNames: ['Route'],
message: 'Please use @kbn/shared-ux-router instead',
},
{
name: 'rxjs/operators',
message:
'Please, use rxjs instead: rxjs/operators is just a subset, unnecessarily duplicating the package import.',
},
];

@Ikuni17
Copy link
Contributor

Ikuni17 commented Dec 4, 2024

@clintandrewhall that makes sense! Looking at .eslintrc.js it seems that I can add it to the RESTRICTED_IMPORTS array:

kibana/.eslintrc.js

Lines 195 to 274 in e067fa2

/** Restricted imports with suggested alternatives */
const RESTRICTED_IMPORTS = [
{
name: 'lodash',
importNames: ['set', 'setWith', 'template'],
message:
'lodash.set/setWith: Please use @kbn/safer-lodash-set instead.\n' +
'lodash.template: Function is unsafe, and not compatible with our content security policy.',
},
{
name: 'lodash.set',
message: 'Please use @kbn/safer-lodash-set/set instead',
},
{
name: 'lodash.setwith',
message: 'Please use @kbn/safer-lodash-set/setWith instead',
},
{
name: 'lodash/set',
message: 'Please use @kbn/safer-lodash-set/set instead',
},
{
name: 'lodash/setWith',
message: 'Please use @kbn/safer-lodash-set/setWith instead',
},
{
name: 'lodash/fp',
importNames: ['set', 'setWith', 'assoc', 'assocPath', 'template'],
message:
'lodash.set/setWith/assoc/assocPath: Please use @kbn/safer-lodash-set/fp instead\n' +
'lodash.template: Function is unsafe, and not compatible with our content security policy.',
},
{
name: 'lodash/fp/set',
message: 'Please use @kbn/safer-lodash-set/fp/set instead',
},
{
name: 'lodash/fp/setWith',
message: 'Please use @kbn/safer-lodash-set/fp/setWith instead',
},
{
name: 'lodash/fp/assoc',
message: 'Please use @kbn/safer-lodash-set/fp/assoc instead',
},
{
name: 'lodash/fp/assocPath',
message: 'Please use @kbn/safer-lodash-set/fp/assocPath instead',
},
{
name: 'lodash.template',
message: 'lodash.template is unsafe, and not compatible with our content security policy.',
},
{
name: 'lodash/template',
message: 'lodash.template is unsafe, and not compatible with our content security policy.',
},
{
name: 'lodash/fp/template',
message: 'lodash.template is unsafe, and not compatible with our content security policy.',
},
{
name: 'react-use',
message: 'Please use react-use/lib/{method} instead.',
},
{
name: 'react-router-dom',
importNames: ['Router', 'Switch', 'Route'],
message: 'Please use @kbn/shared-ux-router instead',
},
{
name: '@kbn/kibana-react-plugin/public',
importNames: ['Route'],
message: 'Please use @kbn/shared-ux-router instead',
},
{
name: 'rxjs/operators',
message:
'Please, use rxjs instead: rxjs/operators is just a subset, unnecessarily duplicating the package import.',
},
];

Adding the package to RESTRICTED_IMPORTS is the process afaik. Making sure to note the alternative.

@weronikaolejniczak
Copy link
Author

@elastic/kibana-operations it's been suggested to mark @kbn/ui-theme and @elastic/eui/dist/eui_theme_*.json imports as warning instead of an error (which will be produced when I put them into the RESTRICTED_IMPORTS). We want the developers to be aware of the deprecation but give them time to slowly migrate.

I could put them into an override of the no-restricted-imports rule instead, what do you think?

@Ikuni17
Copy link
Contributor

Ikuni17 commented Dec 10, 2024

@elastic/kibana-operations it's been suggested to mark @kbn/ui-theme and @elastic/eui/dist/eui_theme_*.json imports as warning instead of an error (which will be produced when I put them into the RESTRICTED_IMPORTS). We want the developers to be aware of the deprecation but give them time to slowly migrate.

I could put them into an override of the no-restricted-imports rule instead, what do you think?

@weronikaolejniczak Creating an override with a warning is correct.

@weronikaolejniczak
Copy link
Author

weronikaolejniczak commented Dec 11, 2024

@Ikuni17 it's not possible in ESLint to define multiple error levels for a rule at once. A workaround is cloning no-restricted-imports and setting it to "warn". Because in EUI we have an ESLint plugin and this Kibana config extends the recommended settings, I'll do it there. I'll come back with another PR for bumping the plugin version and extending the rule pattern set by @kbn/ui-theme.

For now, at the very least, I'd merge this @deprecated change so that developers can already benefit from some sort of euiThemeVars highlighting.

Let me know how that sounds for you!

cc @JasonStoltz

@weronikaolejniczak
Copy link
Author

@Ikuni17 @clintandrewhall I'd appreciate a decision on this one so we can close it 🙏🏻

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 release_note:skip Skip the PR/issue when compiling release notes skip-ci
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate euiThemeVars
4 participants