Babel next plumbing #217
Babel next plumbing #217
Conversation
itsdouges
force-pushed
the
babel-next
branch
2 times, most recently
from
cb37215 to
90abb4a
Compare
itsdouges
added
new feature 🆕
|
|
||
| visitStyledPath(path, state); | ||
| }, | ||
| JSXOpeningElement(path, state) { |
There was a problem hiding this comment.
@Madou: Does spread operator works here 🤔? Something like this:
import '@compiled/css-in-js';
const props = {
css: 'font-size:12px;'
}
const MyDiv = () => {
return <div {...props}>hello</div>
};
There was a problem hiding this comment.
Can't find JSXSpreadAttribute anywhere 🤔
There was a problem hiding this comment.
hi - yeah currently that isn't supported. i don't see that as a very common use case. do you think it's useful to add it in?
my thoughts: do the bare minimum and then add more if it has wide spread usage.
needing a static css prop directly on the jsx element seems like the right path for me.
There was a problem hiding this comment.
Nah!! not that much important for css prop tbh. But yups for other props we should definitely consider it.
my thoughts: do the bare minimum and then add more if it has wide spread usage.
^^ I am sold 😛
| if (t.isIdentifier(expression) && state.declarations) { | ||
| const declaration = state.declarations[expression.name]; | ||
| if (t.isVariableDeclaration(declaration) && declaration.kind === 'const') { | ||
| // We only want to pick out variable declarations that are constants |
There was a problem hiding this comment.
might be an obvious answer - but why? :)
There was a problem hiding this comment.
lets add some more context around why we are doing this.
| path.node.specifiers = path.node.specifiers | ||
| .filter( | ||
| (specifier) => | ||
| specifier.local.name !== 'styled' && specifier.local.name !== 'ClassNames' |
There was a problem hiding this comment.
Okay so no matter what import you use (i.e. what flavour of CSS-in-JS you like) we are going to filter out that import and replace it with CC and CS
| @@ -0,0 +1,164 @@ | |||
| import * as t from '@babel/types'; | |||
| import kebabCase from '@compiled/ts-transform-css-in-js/dist/utils/kebab-case'; | |||
| import { addUnitIfNeeded } from '@compiled/ts-transform-css-in-js/dist/utils/css-property'; | |||
There was a problem hiding this comment.
All these utils in the ast and css builders coming from the TS Transform package feels like an interesting package boundary to me. I guess it's an implementation detail that doesn't impact users at all so it doesn't matter too much, it's only a dev dependency so it doesn't really matter if installing the babel plugin also installs the TS transformer.
|
Remember when I mentioned extracting internals to another package? This is
what I was talking about 😁
…On Mon, 10 Aug 2020, 5:29 pm Jack Lo Russo, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/babel-plugin/src/utils/css-builders.tsx
<#217 (comment)>
:
> @@ -0,0 +1,164 @@
+import * as t from ***@***.***/types';
+import kebabCase from ***@***.***/ts-transform-css-in-js/dist/utils/kebab-case';
+import { addUnitIfNeeded } from ***@***.***/ts-transform-css-in-js/dist/utils/css-property';
All these utils in the ast and css builders coming from the TS Transform
package feels like an interesting package boundary to me. I guess it's an
implementation detail that doesn't impact users at all so it doesn't matter
too much, it's only a dev dependency so it doesn't really matter if
installing the babel plugin also installs the TS transformer.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#217 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABT4PHOT2C2YBERR5BEPZG3R76OV5ANCNFSM4N3A4YLA>
.
|
| return; | ||
| } | ||
|
|
||
| const declarationName = path.node.declarations[0].id.name; |
There was a problem hiding this comment.
add comment explaining why we are doing this and potential pitfalls of the current implementation (name clashes)
| const key = prop.key.name || prop.key.value; | ||
| let value = ''; | ||
|
|
||
| if (t.isStringLiteral(propValue)) { |
There was a problem hiding this comment.
lets add comments with example code for each block to help visualize whats happening
| return value; | ||
| }; | ||
|
|
||
| const extractObjectExpression = (node: t.ObjectExpression, state: State): CSSOutput => { |
There was a problem hiding this comment.
add jsdoc comments to each method explaining what its responsibility is for
|
|
||
| node.properties.forEach((prop) => { | ||
| if (t.isObjectProperty(prop)) { | ||
| // Don't use prop.value directly as it extracts constants from identifiers if needed. |
There was a problem hiding this comment.
make this comment better
| // eslint-disable-next-line @typescript-eslint/no-use-before-define | ||
| const result = extractTemplateLiteral(propValue, state); | ||
| value = result.css; | ||
| variables = variables.concat(result.variables); |
There was a problem hiding this comment.
lets add a comment saying why we arent treating this like a mixin
| variables.push({ name: variableName, expression: propValue }); | ||
| value = `var(${variableName})`; | ||
| } else { | ||
| throw new Error(`Not supported.`); |
There was a problem hiding this comment.
change this to use a stack frame error (maybe in next PR)
| variables = variables.concat(result.variables); | ||
| return; | ||
| } else { | ||
| throw new Error(`Declaration for ${prop.argument.name} could not be found.`); |
There was a problem hiding this comment.
see if we can throw stack frame error (in next PR)
| return extractObjectExpression(node, state); | ||
| } | ||
|
|
||
| throw new Error('Unsupported node.'); |
There was a problem hiding this comment.
throw stack frame error (in another PR)
Ok confirmed we're going with a Babel plugin. We need to get this PR into a shape we're happy to merge and then we'll start working on features in styled components API.