emotion-js / emotion

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

autoprefix object styles at compile time. #92

Closed tkh44 closed 7 years ago

tkh44 commented 7 years ago

This will get us started on the long road of optimizing objects.

I'm thinking we can just detect the first arg of css is array or object then we scan the properties and prefix them. This is going to be a hard problem I think but it will be worth it long term.

threepointone commented 7 years ago

Would actually not recommend we do this. The reasoning is pretty simple - it's impossible to statically detect all usages, which means you'll have unexpected behavior. Would rather recommend to peeps to use their own prefixer, documented well, or use something preval, or use something like glamor/cxs/whatbnot.

nishp1 commented 7 years ago

The reasoning is pretty simple - it's impossible to statically detect all usages

Could you provide an example? Only ones I can thinking of not being able to detect statically are ones that are need runtime (or something like preval or prepack).

import { constants } from './theme'
styled.h1({ transition: contants.fadeTransition })
threepointone commented 7 years ago

Take the example you just gave. Are you going to import it in node(i.e. - eval)? What if there are side effects there? What if it depends on a dom api? Etc etc.

nishp1 commented 7 years ago

FWIW, we do the same for Template literals https://github.com/tkh44/emotion/blob/master/src/parser.js#L84, and would have same issues. I would personally prefer that we add a note in README to help avoid this. Having said that feel free to close PR and issue, if this something you feel should not be supported for object props. :)

Side note, I think if there were a babel plugin that inlines imported values from modules without needing a runtime to avoid side effects you mentioned that would be great for a lot of CSS in JS frameworks allowing us to do a lot of heavy lifting at build time. I may start playing around with that later on but may get complicated due to different variants of module resolution that webpack supports.

tkh44 commented 7 years ago

What @threepointone brings up is very valid.

Maybe a compromise would be to throw warnings when we cant resolve the value?

nishp1 commented 7 years ago

That sounds reasonable to me. How shall we log? Log to terminal console (could easily be missed) or modify AST to add console.warn which would show up in browser Console, or both perhaps?

jaredly commented 7 years ago

I'm confused why y'all prefix the template strings, but not the objects. Would love for objects to not be second-class citizens

threepointone commented 7 years ago

Because that can be done at compilation time easily. Else we have to ship the prefixer in the bundle.

threepointone commented 7 years ago

If you want to use objects badly and don't mind a bit more in your runtime, use glamor/ous!

jaredly commented 7 years ago

hmmm but why fragment the ecosystem? I'd love to be able to set some configuration to either

threepointone commented 7 years ago

I don't consider it fragmentation really. Each option has trade offs, and they all just generate classes and components, and you can use most of them in tandem. Better that where each one has some things they're great at, than something that tries to do everything and isn't good at anything.

Ofc this is just my opinion, kye and mitchell's call in the end.

jaredly commented 7 years ago

I think it's better in the long run for us to settle down, then we only have to fix bugs once etc. Let's at least have a shared core that everyone builds off of instead of reimplementing everything a bunch

threepointone commented 7 years ago

too early for that. Neither glamor nor emotion would have come to life if they were held back by other libs. And there's a lot to come in the future.

nishp1 commented 7 years ago

Because that can be done at compilation time easily.

@threepointone I am pretty sure we can do the same for objects. Problems you and I both mentioned regarding dynamic values are valid for templates as well and emotion won't be able to prefix all cases.

import { constants } from './theme'
styled.h1(`
  transition: ${contants.fadeTransition};
`)

What am I missing?

threepointone commented 7 years ago

Right, because it's interpolated here. My point still stands.

jaredly commented 7 years ago

hmmm I'm not sure the point stands. You're saying we should autoprefix the templates (even though we can't do it perfectly all the time), and not autoprefix objects, because we can't do it perfectly all the time?

threepointone commented 7 years ago

We can do content literals perfectly every time, but not interpolated values (this was mentioned specifically as a con in the glam docs, not here though). It's many times harder wth objects. I learnt and experienced this in depth when doing glamor.

threepointone commented 7 years ago

I mean, if it was so simple, or even good for most cases, wouldn't have bothered with template literals in the first place https://twitter.com/threepointone/status/882550567700180993

nishp1 commented 7 years ago

We can do content literals perfectly every time, but not interpolated values (this was mentioned specifically as a con in the glam docs, not here though). It's many times harder wth objects. I learnt and experienced this in depth when doing glamor.

Could you provide example code of such instances that can be done with content literals perfectly but not object syntax? I am more than happy to tackle them.

threepointone commented 7 years ago

literally the first example you gave me, for the reasons I already mentioned.

import { constants } from './theme'
styled.h1({ transition: contants.fadeTransition })

I'm bailing from this thread, cheers.

nishp1 commented 7 years ago

But that is not doable for templates either is the point @jaredly and I are trying to make.

threepointone commented 7 years ago

wait, i'll type a better example

threepointone commented 7 years ago
// fragments.js
export const bold = css`
  display: flex;
  font-weight: bold
` 
console.log(document.body.innerHTML.length) // or any sideeffectful dom api

// app.js
import { bold } from './fragments'
const H1 = styled.h1`
  composes: ${bold};
  color: red;
`

note that the fragment can be processed, and is statically marked with a fragment-* class (or css-* in emotion) so it can be recognised when it finally merges in runtime.

threepointone commented 7 years ago

I want to be clear, this isn't an opinion. It's just the nature of it, the tradeoff for static vs dynamic code. Ok, bailing thread now :)

nishp1 commented 7 years ago

Open PR (https://github.com/tkh44/emotion/pull/101) already support this.

// fragments.js
export const bold = css({
  display: 'flex',
  font-weight: 'bold'
})
console.log(document.body.innerHTML.length) // or any sideeffectful dom api

// app.js
import { bold } from './fragments'
const H1 = styled('h1')(
  bold, 
  { color: 'red' }
)

// app.js import { bold } from './fragments' const H1 = styled('h1')( bold, { color: 'red' } )



- In a followup PR, we can evaluate the object expression fully as well to and replace it with generated classNames `(css-*)` at build time which bring feature parity with templates and same perf.
threepointone commented 7 years ago

Ah, you're expecting all the objects to have a css() wrap? This is just to support another syntax for css() then really. And won't work when people use polished etc. You better have guards for when people use dynamic objects and throw an error then. The readme/launch post imply that you can use any objects (And it's what people on twitter were discussing too).

jaredly commented 7 years ago

Yeah! That's all I want - to be able to use normal javascript object syntax instead of custom css template syntax. I'm not trying to avoid calling css

jfrolich commented 7 years ago

I also like the object notation more. Would be great if we can have a non-dynamic version of this just as a different notation (not adding additional features). This will also make it easier to convert existing code from glamor(ous) to emotion.

tkh44 commented 7 years ago

I'm waiting on #138. We are going to add it I just want to get proper object merging in first.