Easy to misuse `import {css} from '@emotion/css'` with React components

[Dosant]

I would like to bring up this issue I noticed that it is very easy to misuse emotions's css function together with React components css prop.

Here is the correct usage:

import { css } from '@emotion/react'

render(
  <div
    css={css`
      background-color: hotpink;
    `}
 / >
)

And here is the mistake I've made:

import { css } from '@emotion/css'

render(
  <div
    css={css`
      background-color: hotpink;
    `}
 / >
)

The difference is that I've by mistake imported the css helper from the @emotion/css instead of the @emotion/react. In this case there was no build or runtime warnings and the style just silently didn't apply. Typescript didn't notice any issues, because as far as typescript concerned, both usages are valid.

When I realized that the styles didn't apply, it was very difficult to the root cause and I've spent quite a bit of time trying to figure this out. I suspect that I am not the first one and won't be the last one.

The correct usage of the import { css } from '@emotion/css' is the following:

import { css } from '@emotion/css'

render(
  <div
    className={css`
      background-color: hotpink;
    `}
 / >
)

The difference between @emotion/css and @emotion/react is that the first one is framework agnostic, and the second one is specifically for React.

I noticed a bunch of existing incorrect usages of import { css } from '@emotion/css, which means these styles don't apply:

https://github.com/elastic/kibana/blob/c9eebf262c65d6c3104592b6ea0bb61f1d6e0d1f/x-pack/plugins/synthetics/public/apps/synthetics/components/monitor_details/monitor_status/monitor_status_legend.tsx#L28-L32 cc @elastic/uptime

https://github.com/elastic/kibana/blob/c9eebf262c65d6c3104592b6ea0bb61f1d6e0d1f/src/plugins/unified_search/public/dataview_picker/explore_matching_button.tsx#L38-L41 cc @elastic/kibana-visualizations @elastic/kibana-visualizations-external

https://github.com/elastic/kibana/blob/c9eebf262c65d6c3104592b6ea0bb61f1d6e0d1f/x-pack/plugins/synthetics/public/apps/synthetics/components/monitor_details/monitor_status/monitor_status_header.tsx#L51-L53 cc @elastic/uptime

https://github.com/elastic/kibana/blob/c9eebf262c65d6c3104592b6ea0bb61f1d6e0d1f/x-pack/plugins/security_solution/public/management/pages/policy/view/ingest_manager_integration/endpoint_policy_create_multi_step_extension.tsx#L50-L53 cc @elastic/security-onboarding-and-lifecycle-mgt

https://github.com/elastic/kibana/blob/c9eebf262c65d6c3104592b6ea0bb61f1d6e0d1f/x-pack/plugins/canvas/shareable_runtime/components/rendered_element.tsx#L82-L84 cc @elastic/kibana-presentation

I think we should fix the incorrect usages, but I also would like to discuss if we somehow can prevent this in the future:

• One option could be to forbid imports of the @emotion/css (lint rule), and if you are really sure you need a vanilla version, then you mute the rule. I don't know if there are many valid use-cases for importing @emotion/css instead of @emotion/react as I am not experienced with emotion.
• Better option could be to have a smarter lint rule that errors when import {css} from '@emotion/css' is used together with css prop. I didn't find an existing rule, so it probably needs to be implemented.
• Another option we could try is to globally augment the types so that typescript complained if you'd use css from @emotion/css with emotion's css prop

Dear @elastic/eui-design team, as you have the most experience with emotion, I wonder if you hit this issue before and could weigh in? cc @elastic/kibana-operations because the solution to mitigate is likely a new lint rule

[miukimiu]

Hi @Dosant.

Once @andreadelrio asked us in our slack channel the following:

Hi team. I’m reviewing a PR (still in draft) in Kibana which uses Emotion and the devs are passing the style through the className attribute. I see that in EUI we pass them through the css attribute. Can anybody help me understand a) what’s the difference and b) what would be the recommended way for Kibana?

This was an answer provided by @constancecchen in our EUI slack channel:

In general, we recommend @emotion/react and the css prop over @emotion/css. Emotion automatically handles merging/concatenating all Emotion css for us this way. @emotion/css(docs link) and direct use of className we tend to reserve for non-React scenarios, where you have to pass in a static className (e.g. to a vanilla JS DOM node).

So yes, some of the @elastic/eui-design hit this issue before. I agree that in a React app we should enforce the use of @emotion/react.

Better option could be to have a smarter lint rule that errors when import {css} from '@emotion/css' is used together with css prop. I didn't find an existing rule, so it probably needs to be implemented.

This sounds like a good option. @constancecchen created some eslint rules for our EUI project to improve our Emotion usage and it is working well. 👍🏽

[clintandrewhall]

Another option we could try is to globally augment the types so that typescript complained if you'd use css from @emotion/css with emotion's css prop

I proposed and am leaning toward this option, perhaps in combination with others, as it would explicitly prevent the combination we're trying to avoid. I'm not sure if it's possible, though, but I'm playing with it. Will report back.

[clintandrewhall]

I was able to fix this with a TS type, which we could add to @kbn/ambient-ui-types. We were talking about adding css-prop to that package anyway, this could fix both problems.

Typescript Playground

[spalger]

Feels like this would be relatively easy to write an ESLint rule for