-
Notifications
You must be signed in to change notification settings - Fork 261
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
[wip] Transpile external css #100
Conversation
@rauchg @nkzawa early feedbacks are appreciated, what do you think about the syntax? should we stick with the |
|
I am going for
We don't have to it should happen automatically because the external stylesheet is a regular js module. Not sure if I am missing something though but I will able to tell soon – I am half way through implementing the import injection and style tag replacement :) |
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.
need to think if this solution could work at all since the hash computation could not be possible with this approach
return attr && attr.get('value').node.value | ||
} | ||
|
||
const getImport = ([styleText, styleId, importPath]) => ( |
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.
need to fix this (and maybe call it makeImportExternal
) because transpiled (external) stylesheets are exporting a single object (default)
I thought a little bit about this approach and I think that it could work in case we are cool with allowing only a single style tag when using external css: /* valid */
export default () => (
<p>
test
<style jsx src="./style.js" />
</p>
)
/* not valid */
export default () => (
<p>
test
<style jsx>{`p { font-size: 3em }`}</style>
<style jsx src="./style.js" />
</p>
) This is because I would use the resolved @nkzawa @rauchg if you like this solution and think that this feature could add value to the library I will finalize it and put in a mergeable state. |
I think it can work even if there are multiple style elements since we already support with |
export default ( | ||
<style jsx>{` | ||
p { color: ${foo} } | ||
`}</style> |
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.
Does this syntax mean we can't import others from imported styles ? 🤔
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.
Why don't we simply for a template string like:
const foo = 'red'
export default `
p { color: ${foo} }
`
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.
This would be the best api however is not that easy to achieve this because in the consumer file (the one that imports this) we need to hash `p { color: ${foo} }`
and once we have the hash we need to transform the css (add the prefix to it etc).
when external files are transpiled they don't have knowledge of the consumer component therefore when transforming the css we don't have all of the necessary information to create a proper hash (jsxId). /* external file */
`p { color: red }` The /* consumer */
`p { font-size: 3em }` // own styles
src="./style.js" The i.e.
Uh this is kinda tricky, I am using that syntax so that I can tell whether a module is an external styled-jsx sheet. If we don't do so then we'd need a separate plugin to transpile external files. |
The only way I can think of to allow multiple style elements is to transpile external css like so: p[data-jsx~="woot"] { color: red } (notice the and the consumer eventually will be: p[data-jsx~="wat"] { font-size: 3em } and <p data-jsx="woot wat">test</p> |
The other thing I've been thinking about is that we could have "flow" analysis. This would work: import style from './style'
export default (
<div>
<p>hi</p>
<style jsx>{style}</style>
</div>
) and this would work: cons style = `
p { color: red }
`
export default (
<div>
<p>hi</p>
<style jsx>{style}</style>
</div>
) |
🤔 I thought about case 1 for external files but in order to do so we'd need to hash using the resolved file name. Also when transpiling an external file we need a way to tell if that's just an external stylesheet or any other module/react component. That's why in my wip I export JSX (a style tag). |
Why can't we just hash based on contents? The idea is that by analyzing the "flow" we can access the raw string exactly the same as if it was inlined inside |
It also seems like a natural complement of the work we've been doing to allow constants inside the source |
@rauchg based on .jsx syntax, my first reaction was to automatically assume that you could do this |
@tgoldenberg all of those problems are tractable. We tend to ship and iterate. |
Also, whenever we go against intuition, we attempt to provide very clear error messages: for example, if you're trying to use prop or state inside the style text |
Don't get me wrong I actually like the idea of flow analysis.
This means that we'd need to read (or eval) external files as we transpile and in general the traversal would get a bit trickier. Also we may need to revisit the expressions logic since at the moment we just replace them with placeholders before hashing (we don't analyse them). In the end though probably this will result in the better api. |
as I said maybe for external files we can hash the resolved file path instead of the content (css) |
fixes #83