Add support for expressions within template literals - #61
Conversation
|
fyi this patch doesn't allow expressions in the selector name like edit: actually it could be useful to define media queries and whatnot. That will require some work on the CSS compiler. |
|
Waiting for this to land. 🚀 |
| @@ -17,23 +18,98 @@ const STYLE_COMPONENT_CSS = 'css' | |||
| export default function ({types: t}) { | |||
| const findStyles = children => ( | |||
| children.filter(el => ( | |||
There was a problem hiding this comment.
Just a small thing, think you can destructure this param ({node}) => ( which allows you to remove el in front of node
There was a problem hiding this comment.
oh right good catch!
|
Are we erroring upon usage of vars from the closure? |
|
No but we could although it might add some overhead or be not that straightforward if we keep allowing things like call expressions |
|
basically you want to make sure that people don't use |
|
Correct. If this doesn't fail it will create extremely difficult-to-understand bugs to the uninitiated: export default ({ some, props }) => (
<div>
<style jsx>{`
div { color: ${some} }
`}</style>
</div>
) |
|
@rauchg any identifier in the scope or just props? export default () => {
const some = 'red' // allowed or not?
return (<div>
<style jsx>{`
div { color: ${some} }
`}</style>
</div>)
} |
|
I'd block anything from the scope. After all, if it's a constant, you should probably syntactically move it outside the scope… |
| if (isTemplateLiteral) { | ||
| // build the expression from transformedCss | ||
| traverse( | ||
| parse(`\`${transformedCss}\``).ast, |
There was a problem hiding this comment.
I think we should use https://github.com/babel/babylon
There was a problem hiding this comment.
babel-core's transform uses babylon behind the wood
There was a problem hiding this comment.
yep, but I meant it would be better to use the parser anyway.
There was a problem hiding this comment.
rofl typo. @nkzawa yea sorry I was rushing. In fact I thought of doing that initially but then I didn't feel like adding the extra dependency but if you think that it is preferable I have no problem with that.
|
Needs rebase |
Conflicts: src/babel.js test/index.js
|
@rauchg you got it |
|
Any updates on this? Waiting to 🚀 |
|
@sarukuku I haven't had a lot of free time recently. Anyway we want to benchmark this before merging – I will try to push a minimal benchmark today after work. |
|
301 to #80 :) |
6 participants
Fixes #25