-
Notifications
You must be signed in to change notification settings - Fork 265
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh right good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use https://github.com/babel/babylon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
babel-core's transform uses babylon behind the wood
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, but I meant it would be better to use the parser anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Behind the wood 😄👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :) |
Fixes #25