-
Notifications
You must be signed in to change notification settings - Fork 262
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 external StyleSheets support #148
Conversation
You guys may want to review per commit since the branch is quite big :) |
@giuseppeg For that linter error you can just use elision: const [
id,
,
externalStylesReference,
isGlobal
] = state.externalStyles.shift() |
Test are passing locally and failing in CI because |
wow. Amazing progress |
@rauchg since babel processes files alphabetically (I believe) external stylesheets are not transpiled in some cases. An example would be To work around this issue we can take one of the following approaches:
Edit I decided that I will parse the string with CSSTree, if parsing / validation fails I'll skip the file – in the future we can add warnings or throw an error. CSSTree seems to be super fast. |
9962373
to
5aa9592
Compare
src/babel-external.js
Outdated
|
||
let globalCss = css.modified || css | ||
|
||
if (validate && !isValidCss(globalCss)) { |
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.
if we want to avoid this we could require people to tag external stylesheets like so:
export default styledJsx`
div { color: red }
`
I wonder if there's a way to invoke babel from within the plugin to work around this limitation? As long as caching is enabled, it would incur in overhead for the user (double parsing) |
@rauchg I am not sure if 1) plugins get the dest path 2) if we write the file to disk after transforming it with babel-core (like we do for tests) that would conflict or be overridden by the original one (when transpiled) |
@rauchg @nkzawa this branch is ready for review, I still need to write some docs and I'll test this branch with next-news. Here is a breakdown of what I did: usageExternal stylesheets are ES6 or CommonJS modules that export a string or template literal. /* styles.js */
export default `div { background-color: red }` /* otherStyles.js */
module.exports = "p { color: white }" Expressions work. Within components these can be referenced like so: import styles from './styles'
const otherStyles = require('./otherStyles')
export default () => (
<div>
<p>hello</p>
<style jsx>{styles}</style>
<style jsx>{otherStyles}</style>
</div>
) Styles can also be consumed as how this worksInstead of hashing the CSS string, I calculate the hash for each external stylesheet by using its absolute path (filename) which is always unique. hash(filaname) Since one may want to use multiple stylesheets I introduced a new data attribute [data-jsx-ext~="a"] { /* ... */ }
[data-jsx-ext~="b"] { /* ... */ } Notice the Meaning that in the markup we will have: <div data-jsx-ext="a b c d"> That said the external stylesheets are rewritten to something like this export default {
global: `div { background-color: red }`,
local: `div[data-jsx-ext~="597123001"] { background-color: red }`
} And the <_JSXStyle styleId={597123001} css={styles.local} />
<_JSXStyle styleId={597123003} css={styles.global} /> implementation detailsYou may want to review things by commit – maybe it'd make it easier to follow what is going on.
Note at this point I was keeping track of the external stylesheets references and transpile those files as I encountered them. However this has proven to be wrong because: depending on the order in which babel processes files some stylesheets might never be compiled. This is due to the fact that
An alternative solution would be to tag the file with a comment
Alternatively probably we can hash the whole file content instead of the filename and remove this issue. If you need any more info you know where to find me :) |
This would add some overhead since we'd need to do i/o (read the file from disk) |
Just a question: Would you, with this, be able to use, eg. import stylesheet from './styles.css'
export default () => (
<div>
<p>hello</p>
<style jsx>{stylesheet}</style>
</div>
) /* styles.css */
p {
color: red;
} |
@thomaslindstrom I am not exactly sure but in theory yes. |
sorry for late replay. I didn't read details of implementations yet, but ideas seem brilliant 👍 |
@nkzawa no worries, I just pinged you because I would like to merge this (if we think that it is a good solution) to avoid crazy merge conflicts (I touched quite a lot) |
import { a, b } from './external-styles'
export default () => (
<div>
<style jsx>{ a } { b }</style>
</div>
) |
No they don't, only
Not right now because of this check, we could add support for that too though. import { a } from './external-styles'
export default () => (
<div>
<style jsx>
{`div { color: red }`}
{ a }
</style>
</div>
) NB. this adds complexity and could be achieved with two |
Looks great! Hope that release soon. |
I'm looking foward to see this PR be merged. |
What are we waiting on to get this merged? Happy to write documentation if that's all that is missing. |
Hold in the excitement folks! :D We'll merge (soon) once this branch has been reviewed and is good to go :) |
Fixes #83
This branch introduces external stylesheets support.
External stylesheets are js modules that export a string or template literal:
Which can be used as follow:
Please read the
Implementation details
Say that you have `components/Card/style.js`:File names are unique so we can generate the hash for external files using the filename
hash(path.resolve('components/Card/style.js'))
and transpile the file like this:the
~
matches a word therefore you can havedata-jsx-ext~="woot wat"
wherewoot
andwat
are external stylesheets hashes.For the sake of supporting both global and local styles the transpiled stylesheet exports an object with two keys
global
andlocal
respectively for the original version (global) and scoped one. This allows people to use the same stylesheet in both modes (maybe it is overkill and we can remove it if you want).TODOs