emotion-js / emotion

👩‍🎤 CSS-in-JS library designed for high performance style composition
https://emotion.sh/
MIT License
17.49k stars 1.11k forks source link

typecheck classnames #2461

Closed alita-moore closed 2 years ago

alita-moore commented 3 years ago

The problem

Having to write out css class name strings is brittle and not lintable

const style = css`
  .class-1 {}
  .class-2 {}
`

<div css={style}>
  <div className={"class-1"}/>
  <div className={"class-2")/>
</div>

if class-1 were to be changed to class-3 or something then the class wouldn't be found but while writing this code there don't appear to be any issues / it's not caught by a linter

Proposed solution

Treat css classes associated with a css instance as an enum / object. This will automatically catch if a string you're using isn't available in the classname

const style = css`
  .class-1 {}
  .class-2 {}
`

<div css={style}>
  <div className={style["class-1"]}/>
  <div className={style["class-2"])/>
</div>

where style["class-1"] === "class-1" but it's equivalent to

const style: { [K in "class-1" | "class-2": string } = { "class-1": "class-1", "class-2": "class-2" }

so if "class-1" -> "class-3" then style["class-1"] is not a valid type and it will be type checked

Alternative solutions

provide a plugin / pre-compile that checks the used classes and errors or at least warns if the class doesn't exist

Additional context

I program for a living and strings are awful / brittle. I adore enums and getting rid of strings altogether is a super useful thing

srmagura commented 3 years ago

Hey Alita, I agree with you on the importance of type safety and this is one of the benefits of using Emotion.

I think you can solve this by changing your code to something like this:

import { css } from '@emotion/react'

const css1 = css({ color: 'blue' })
const css2 = css({ color: 'red' })

const markup = <div>
  <div css={css1} />
  <div css={css2} />
</div>

Now, if you were to accidentally type css3, TypeScript would give you a compile error. Using object styles is also beneficial as they can be typechecked.

alita-moore commented 3 years ago

Hey Alita, I agree with you on the importance of type safety and this is one of the benefits of using Emotion.

I think you can solve this by changing your code to something like this:

import { css } from '@emotion/react'

const css1 = css({ color: 'blue' })
const css2 = css({ color: 'red' })

const markup = <div>
  <div css={css1} />
  <div css={css2} />
</div>

Now, if you were to accidentally type css3, TypeScript would give you a compile error. Using object styles is also beneficial as they can be typechecked.

Yep but in that case it will create a new emotion instance for all divs which I this is non performant. It's also nicer from a dev perspective to use classes.

Is this a challenging feature to implement?

srmagura commented 3 years ago

There may be another solution to your problem that does not require changes to Emotion itself. Could you describe your use case more?

How many divs are there? How much do the styles vary from one div to the next?

alita-moore commented 3 years ago

Oh well I'm just using it as standard css

const style = css`
   .header {
    ...
  }
  .button {
    ...
  }
  .blue-text {
    ...
  }
`
export default App(){
  return (
    <div css={style}>
      <div class="header">
         <span class="blue-text"> header </span>
      </div>
      <div class="button" onClick=() => {}/>
    </div>
  )
}

basically, emotion offers a really powerful use case where you can define classes at lower levels and allow those classes to only be accessible to the child nodes. That then lets you do things very cleanly. It's also how css is normally used without emotion. This is also useful because of things like css packs where you want to use class names in conjunction with each other (bootstrap etc.). you could do it like you suggested, but it's easier to read the initial style declaration and its easier on the dom.

srmagura commented 3 years ago

That makes sense. It is a smart, if not traditional, way to use CSS-in-JS.

That said, I don't think it makes sense to implement this in the Emotion core. I'm not sure how many people would benefit from this change. And, the more I think about it, I'm not sure if this is even possible to implement from a TypeScript perspective since TypeScript would have to infer the type of style from the string passed to css.

You could keep your code largely unchanged while improving type safety by defining a class name enum:

enum ClassName {
    Header = 'header',
    Button = 'button',
    BlueText = 'blue-text'
}
alita-moore commented 2 years ago

@srmagura I spent some time using the enum approach in my production code. It works but requires a lot of boiler plate that gets out of sync quickly. It also makes it annoying to delete things because in order to delete I have to also delete in the enum. This seems trivial but it wastes a lot of time.

In general implementing this would offer

  1. Cleaner code
  2. Easier to manage class name editing

Which would save a lot of time, money, and make code more readable

alita-moore commented 2 years ago

Also you can have a scenario where a enum had a class but the css doesn't or the strings aren't in sync and there won't be any type checking to notice that. So that'll also lead to much more time spent on debugging

srmagura commented 2 years ago

Closing because I believe this is out of scope for Emotion. It is a cool idea so let us know if you develop something to fit this use case.