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

Enable CSS-in-JS styling with emotion #98157

Merged
merged 37 commits into from
Jul 9, 2021
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
778627a
emotion deps
thompsongl Apr 22, 2021
196bf60
kbn-babel
thompsongl Apr 22, 2021
b60a64e
kbn-test
thompsongl Apr 22, 2021
d596827
examples
thompsongl Apr 22, 2021
db03924
babel-plugin-styled-components config
thompsongl Apr 23, 2021
fd81944
Merge branch 'master' into 95557-emotion
thompsongl Apr 23, 2021
014f2f0
css prop type fixes
thompsongl Apr 26, 2021
f457a73
type context
thompsongl Apr 26, 2021
aceaed2
Merge branch 'master' into 95557-emotion
thompsongl Apr 26, 2021
255a812
declaration location
thompsongl Apr 26, 2021
df51fff
some emotion types resolved
thompsongl Apr 28, 2021
d815ad9
clean up
thompsongl Apr 28, 2021
59ac889
emotion v10 accomodations
thompsongl Apr 29, 2021
c11ebb5
types
thompsongl Apr 30, 2021
361e2f1
Merge branch 'master' into 95557-emotion
thompsongl May 5, 2021
3bfddfd
kbn-crypto
thompsongl May 5, 2021
c877306
Merge branch 'master' into 95557-emotion
thompsongl Jun 21, 2021
cd45fda
kbn-telemetry-tools
thompsongl Jun 21, 2021
6b3d432
Merge branch 'master' into 95557-emotion
thompsongl Jun 21, 2021
b008566
bazel
thompsongl Jun 21, 2021
f811056
Merge branch 'master' into 95557-emotion
thompsongl Jun 28, 2021
985577b
Merge branch 'master' into 95557-emotion
thompsongl Jun 30, 2021
1dda574
Merge branch 'master' into 95557-emotion
thompsongl Jun 30, 2021
c777224
eslint rule; shared file regex array
thompsongl Jun 30, 2021
3972208
update paths
thompsongl Jul 6, 2021
8bdfd6b
Merge branch 'master' into 95557-emotion
thompsongl Jul 6, 2021
cc91272
Update packages/kbn-eslint-plugin-eslint/rules/module_migration.js
thompsongl Jul 6, 2021
f6a1e7e
remove placeholder styles
thompsongl Jul 6, 2021
47455f1
Merge branch 'master' into 95557-emotion
thompsongl Jul 6, 2021
4a93b5b
doc api changes
thompsongl Jul 6, 2021
b5d2633
snapshot updates
thompsongl Jul 7, 2021
a99dc6a
storybook comments
thompsongl Jul 7, 2021
145a606
use constant
thompsongl Jul 8, 2021
7547d66
Merge branch 'master' into 95557-emotion
thompsongl Jul 8, 2021
cbf4c94
bump new deps
thompsongl Jul 8, 2021
b464828
condense versions
thompsongl Jul 8, 2021
447e80e
Merge branch 'master' into 95557-emotion
thompsongl Jul 9, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@
"@elastic/safer-lodash-set": "link:bazel-bin/packages/elastic-safer-lodash-set",
"@elastic/search-ui-app-search-connector": "^1.5.0",
"@elastic/ui-ace": "0.2.3",
"@emotion/react": "^11.1.5",
"@hapi/accept": "^5.0.2",
"@hapi/boom": "^9.1.1",
"@hapi/cookie": "^11.0.2",
Expand Down Expand Up @@ -452,6 +453,8 @@
"@elastic/eslint-plugin-eui": "0.0.2",
"@elastic/github-checks-reporter": "0.0.20b3",
"@elastic/makelogs": "^6.0.0",
"@emotion/babel-preset-css-prop": "^11.2.0",
"@emotion/jest": "^11.2.0",
"@istanbuljs/schema": "^0.1.2",
"@jest/reporters": "^26.6.2",
"@kbn/babel-code-parser": "link:bazel-bin/packages/kbn-babel-code-parser",
Expand Down
38 changes: 30 additions & 8 deletions packages/kbn-babel-preset/webpack_preset.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@
* Side Public License, v 1.
*/

const usesStyledComponents = [
/src[\/\\]plugins[\/\\](data|kibana_react)[\/\\]/,
/x-pack[\/\\]plugins[\/\\](apm|beats_management|fleet|infra|lists|observability|osquery|security_solution|uptime)[\/\\]/,
/x-pack[\/\\]test[\/\\]plugin_functional[\/\\]plugins[\/\\]resolver_test[\/\\]/,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a useful error message if a developer tries to use styled-components outside these directories?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not currently, but if the ops folks agree that this opt-in/opt-out approach is the right way to go, we can look at adding a message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the plan to discouraging use of styled-components and push the teams working on these plugins to migrate to emotion?

What if we added an eslint error message for importing styled-components that told people to use @emotion/react instead. Then people would need to disable the eslint rule when importing it which I think is ideal if we're actively discouraging the spread of styled-compononents. We could also have the opposite rule in place and maintain this list of selectors in .eslintrc.js or something so they stay synchronized.

(This is quite a list of plugins already using styled-components though, didn't expect it to be so large)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the plan to discouraging use of styled-components and push the teams working on these plugins to migrate to emotion?

Yes. The APIs are similar enough that migration shouldn't be time consuming. I was also surprised to see so many plugins using it; the list has grown since I last did an audit.

Good ideas regarding eslint. I'll look in to creating a rule and some kind of synchronizable list.

Copy link
Contributor

@spalger spalger May 24, 2021

Choose a reason for hiding this comment

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

Great, I think since we're going to need to scope this to specific files we might want to customize the @kbn/eslint/module_migration rule: https://github.com/elastic/kibana/blob/master/packages/elastic-eslint-config-kibana/.eslintrc.js#L37-L41

We can add file globs to include/exclude specific migration mappings for specific patterns using regex like above. Should be as simple as filtering the list of mappings and then returning an empty visitor {} when the mappings are empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we create a meta issue with an explicit deadline to track the migration process?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally got back to this. Extended @kbn/eslint/module_migration to accept include and exclude and used that to write a new rule for styled-components. The file regex array used by @kbn/babel-preset is now stored in @kbn/dev-utils and is shared across packages.

];

module.exports = () => {
return {
presets: [
Expand All @@ -21,14 +27,6 @@ module.exports = () => {
],
require('./common_preset'),
],
plugins: [
[
require.resolve('babel-plugin-styled-components'),
{
fileName: false,
},
],
],
env: {
production: {
plugins: [
Expand All @@ -42,5 +40,29 @@ module.exports = () => {
],
},
},
overrides: [
{
include: usesStyledComponents,
plugins: [
[
require.resolve('babel-plugin-styled-components'),
{
fileName: false,
},
],
],
},
{
exclude: usesStyledComponents,
presets: [
[
require.resolve('@emotion/babel-preset-css-prop'),
{
labelFormat: '[local]',
},
],
],
},
],
};
};
3 changes: 2 additions & 1 deletion packages/kbn-crypto/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ TYPES_DEPS = [
"@npm//@types/node",
"@npm//@types/node-forge",
"@npm//@types/testing-library__jest-dom",
"@npm//resize-observer-polyfill"
"@npm//resize-observer-polyfill",
"@npm//@emotion/react",
Copy link
Member

Choose a reason for hiding this comment

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

@mistic does this make sense to you? Or do we think it's one of dependency ordering issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was some related discussion in #99382 (comment)
Basically, modifying tsconfig.base.json makes this a dependency almost everywhere

Copy link
Member

Choose a reason for hiding this comment

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

yeah it makes sense as we are inheriting from the tsconfig.base.json here and we don't override the type definitions in tsconfig file of that package. probably we should do it

]

DEPS = SRC_DEPS + TYPES_DEPS
Expand Down
17 changes: 16 additions & 1 deletion packages/kbn-storybook/lib/default_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@
* Side Public License, v 1.
*/

import * as path from 'path';
import { StorybookConfig } from '@storybook/core/types';

const toPath = (_path: string) => path.join(process.cwd(), _path);
thompsongl marked this conversation as resolved.
Show resolved Hide resolved
export const defaultConfig: StorybookConfig = {
addons: ['@kbn/storybook/preset', '@storybook/addon-a11y', '@storybook/addon-essentials'],
stories: ['../**/*.stories.tsx'],
Expand All @@ -22,6 +24,19 @@ export const defaultConfig: StorybookConfig = {

config.node = { fs: 'empty' };

return config;
const emotion11CompatibleConfig = {
...config,
resolve: {
...config.resolve,
alias: {
...config.resolve?.alias,
'@emotion/core': toPath('node_modules/@emotion/react'),
'@emotion/styled': toPath('node_modules/@emotion/styled'),
'emotion-theming': toPath('node_modules/@emotion/react'),
},
},
};

return emotion11CompatibleConfig;
},
};
3 changes: 2 additions & 1 deletion packages/kbn-telemetry-tools/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ TYPES_DEPS = [
"@npm//@types/node",
"@npm//@types/normalize-path",
"@npm//@types/testing-library__jest-dom",
"@npm//resize-observer-polyfill"
"@npm//resize-observer-polyfill",
"@npm//@emotion/react",
]

DEPS = SRC_DEPS + TYPES_DEPS
Expand Down
1 change: 1 addition & 0 deletions packages/kbn-test/jest-preset.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ module.exports = {
snapshotSerializers: [
'<rootDir>/src/plugins/kibana_react/public/util/test_helpers/react_mount_serializer.ts',
'<rootDir>/node_modules/enzyme-to-json/serializer',
'<rootDir>/node_modules/@emotion/jest/serializer',
],

// The test environment that will be used for testing
Expand Down
1 change: 1 addition & 0 deletions packages/kbn-ui-shared-deps/src/entry.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export const KbnI18n = require('@kbn/i18n');
export const KbnI18nAngular = require('@kbn/i18n/angular');
export const KbnI18nReact = require('@kbn/i18n/react');
export const Angular = require('angular');
export const EmotionReact = require('@emotion/react');
export const Moment = require('moment');
export const MomentTimezone = require('moment-timezone/moment-timezone');
export const KbnMonaco = require('@kbn/monaco');
Expand Down
1 change: 1 addition & 0 deletions packages/kbn-ui-shared-deps/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ exports.externals = {
'@kbn/i18n': '__kbnSharedDeps__.KbnI18n',
'@kbn/i18n/angular': '__kbnSharedDeps__.KbnI18nAngular',
'@kbn/i18n/react': '__kbnSharedDeps__.KbnI18nReact',
'@emotion/react': '__kbnSharedDeps__.EmotionReact',
jquery: '__kbnSharedDeps__.Jquery',
moment: '__kbnSharedDeps__.Moment',
'moment-timezone': '__kbnSharedDeps__.MomentTimezone',
Expand Down
7 changes: 7 additions & 0 deletions src/core/public/chrome/ui/header/collapsible_nav.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { groupBy, sortBy } from 'lodash';
import React, { Fragment, useRef } from 'react';
import useObservable from 'react-use/lib/useObservable';
import * as Rx from 'rxjs';
import { css } from '@emotion/react';
thompsongl marked this conversation as resolved.
Show resolved Hide resolved
import { ChromeNavLink, ChromeRecentlyAccessedHistoryItem } from '../..';
import { AppCategory } from '../../../../types';
import { InternalApplicationStart } from '../../../application/types';
Expand Down Expand Up @@ -113,6 +114,10 @@ export function CollapsibleNav({
});
};

const styles = css`
background: coral;
`;

return (
<EuiCollapsibleNav
data-test-subj="collapsibleNav"
Expand All @@ -123,6 +128,7 @@ export function CollapsibleNav({
isOpen={isNavOpen}
isDocked={isLocked}
onClose={closeNav}
css={styles}
>
{customNavLink && (
<Fragment>
Expand Down Expand Up @@ -187,6 +193,7 @@ export function CollapsibleNav({
color="text"
gutterSize="none"
size="s"
css={{ background: 'coral' }}
/>
</EuiCollapsibleNavGroup>
</EuiFlexItem>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/

import React, { useCallback, useMemo, useState } from 'react';
import { css } from '@emotion/react';
import { FormattedMessage } from '@kbn/i18n/react';
import './discover_grid.scss';
import {
Expand Down Expand Up @@ -150,8 +151,12 @@ export interface DiscoverGridProps {
controlColumnIds?: string[];
}

const styles = css`
background: coral;
`;

thompsongl marked this conversation as resolved.
Show resolved Hide resolved
export const EuiDataGridMemoized = React.memo((props: EuiDataGridProps) => {
return <EuiDataGrid {...props} />;
return <EuiDataGrid {...props} css={styles} />;
});

export const DiscoverGrid = ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
"types": [
"node",
"jest",
"react"
"react",
"@emotion/react/types/css-prop"
]
},
"include": [
Expand Down
2 changes: 1 addition & 1 deletion test/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
"emitDeclarationOnly": true,
"declaration": true,
"declarationMap": true,
"types": ["node", "resize-observer-polyfill"]
"types": ["node", "resize-observer-polyfill", "@emotion/react/types/css-prop"]
},
"include": [
"**/*",
Expand Down
8 changes: 6 additions & 2 deletions tsconfig.base.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@
// Allows for importing from `kibana` package for the exported types.
"kibana": ["./kibana"],
"kibana/public": ["src/core/public"],
"kibana/server": ["src/core/server"]
"kibana/server": ["src/core/server"],
"@emotion/core": [
"typings/@emotion"
],
Comment on lines +16 to +18
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stubs the @emotion/core types, which are only used internally by Storybook, but cause type errors if included.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe Storybook should use a dedicated tsconfig.json instead of polluting the base global config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EUI PRs like this have highlighted the need to make better use of the various tsconfig files, especially for browser vs. node environments. @mistic and @spalger are aware, but I'm not sure if there is an issue tracking the idea.
Note that this is a temporary change that can be reverted when Storybook moves to Emotion 11. I'm happy to make further changes, though, if y'all decide that kbn-storybook should be handled differently.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy to make further changes, though, if y'all decide that kbn-storybook should be handled differently.

If @elastic/kibana-operations team thinks it's acceptable... @mistic any actionable items for the problem?

},
// Support .tsx files and transform JSX into calls to React.createElement
"jsx": "react",
Expand Down Expand Up @@ -62,7 +65,8 @@
"flot",
"jest-styled-components",
"@testing-library/jest-dom",
"resize-observer-polyfill"
"resize-observer-polyfill",
"@emotion/react/types/css-prop"
]
}
}
11 changes: 11 additions & 0 deletions typings/@emotion/index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

// Stub @emotion/core
// Remove when @storybook has moved to @emotion v11
thompsongl marked this conversation as resolved.
Show resolved Hide resolved
export {};
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import React, { FunctionComponent, useState } from 'react';
import PropTypes from 'prop-types';
import { css as emotionCss } from '@emotion/react';
import {
EuiFieldText,
EuiFieldNumber,
Expand Down Expand Up @@ -69,10 +70,14 @@ export const WorkpadConfig: FunctionComponent<Props> = (props) => {
},
];

const styles = emotionCss`
background: coral;
`;

return (
<div>
<div className="canvasLayout__sidebarHeaderWorkpad">
<EuiTitle size="xs">
<EuiTitle size="xs" css={styles}>
<h4>{strings.getTitle()}</h4>
</EuiTitle>
</div>
Expand Down Expand Up @@ -138,7 +143,7 @@ export const WorkpadConfig: FunctionComponent<Props> = (props) => {

<VarConfig variables={variables} setVariables={setWorkpadVariables} />

<div className="canvasSidebar__expandable">
<div className="canvasSidebar__expandable" css={{ color: 'coral' }}>
<EuiAccordion
id="accordion-global-css"
className="canvasSidebar__accordion"
Expand All @@ -152,6 +157,7 @@ export const WorkpadConfig: FunctionComponent<Props> = (props) => {
<span>{strings.getGlobalCSSLabel()}</span>
</EuiToolTip>
}
css={{ color: 'coral' }}
>
<div className="canvasSidebar__accordionContent">
<EuiTextArea
Expand Down
Loading