vercel / styled-jsx

Full CSS support for JSX without compromises
http://npmjs.com/styled-jsx
MIT License
7.7k stars 261 forks source link

Can no longer inject style rule from a const string since version 2 #322

Closed coox closed 6 years ago

coox commented 7 years ago

It seems impossible to inject a style rule from a const string using styled-jsx version 2. Since this feature was supported in version 1, I would consider this a regression.

My personal use case is very simple, I would like to be able to define styles in CSS files to benefit from standard tooling for editing/linting, which are much harder to leverage with inline CSS-in-string or CSS-in-template-literal. I am sure that many other use cases would also be a fit.

Is this a bug? A known issue/limitation? I’d be happy to help either way, provided a little guidance.

Simple demonstration

You can also run a simple use case from this sample project (using Next.js): https://github.com/coox/styled-jsx-unexpected-curly-braces/blob/master/pages/index.js

Inline string: works!

The following rule injection works great, as expected:

<style jsx global>{'body { background-color: green; }'}</style>

Output:

<style id="__jsx-id">body{background-color:green;}</style>

Constant string literal: breaks!

However, just moving the string literal to a constant does not even build:

const greenBodyStyleSheet = 'body { background-color: green; }';
<style jsx global>{greenBodyStyleSheet}</style>

Output:

SyntaxError: The Identifier `greenBodyStyleSheet` is either `undefined` or it is not an external StyleSheet reference i.e. it doesn't come from an `import` or `require` statement`

Interpolated constant string in template literal: puzzles!

The closest I got was trying to interpolate the const inside a template literal. This does build, but the output was not what I was expecting:

const greenBodyStyleSheet = 'body { background-color: green; }';
<style jsx global>{`${greenBodyStyleSheet}`}</style>

Output (notice the unexpected surrounding curly braces):

<style id="__jsx-id">{body { background-color: green; };}</style>
giuseppeg commented 7 years ago

@coox hey there. Thanks for the exhaustive explanation!

Although that pattern worked in v1 it probably was never meant to be like that - mostly because when you use interpolations like that styles are not preprocessed and you don't get vendor prefixes and so on.

Instead you may want to use external styles – a separate javascript module that exports your styles.

To use your example:

// styles.js
import css from 'styled-jsx/css'
export const greenBodyStyleSheet = css`body { background-color: green; }`
// app.js
import { greenBodyStyleSheet } from './styles'

<style jsx global>{greenBodyStyleSheet}</style>

This should work just fine. If you want you could write your styles in CSS files and convert to javascript modules with a script or something before running Next or using Babel.

You can try the following but it might not work because we only look up for expressions coming from imports/requires (in case you can open a new issue to consider adding support for such a feature)

import css from 'styled-jsx/css'
const greenBodyStyleSheet = css`body { background-color: green; }`

<style jsx global>{greenBodyStyleSheet}</style>

The extra curly brackets are definitely a bug although probably it doesn't make sense to use styles like that.

coox commented 6 years ago

Thanks for your detailed response, @giuseppeg!

I would happily use css-tagged template literals in external style modules. However, this is really only going to push the issue of injecting CSS files one step further, with an extra pinch of complexity in the build process.

And unfortunately, it also provides similar results… I have updated my example repo, and even though I apparently benefit from pre-processing when using styled-jsx-css-from-style-sheet-string, the ”extra curly braces“ bug is still happening.

Workaround

So far, the only way I have found to work around this issue is to inject the content whole content of my CSS files within the page using Babel using preval, or rather, a combination of babel-macros and preval.macro, like this:

<style jsx global>{preval`module.exports = require('fs').readFileSync(require.resolve('./style.css'), 'utf8')`}</style>

But this is really unsatisfying, mostly because it prevents hot module reload goodness. I think I’m not the only one aiming at the beautiful Developer Experience described in Next.js’ with-global-stylesheet example, with the benefits from styled-jsx added!

Moving forward

I believe that the solution would involve:

I think that this would make the code that you suggest trying (and I tried!) work.

If you agree, I’d happily open separate issues (with references to the present issue that I’d close), and I’m also happy to help working on them, guidance provided!

Many thanks,

giuseppeg commented 6 years ago

Why this?

import css from 'styled-jsx/css';

const styleSheetString = 'body { background-color: yellow; }';
export default css`${styleSheetString}`;

I am pretty sure that the following will work:

import css from 'styled-jsx/css'
export default css`body { background-color: yellow; }`

An hypothetical build script would do

echo "import css from 'styled-jsx/css'; export default css\``cat test.css`\`" > test.js

Then you let styled-jsx compile it for you - you don't have to do a thing just use the styles:

import testStyles from './test'

<style jsx global>{testStyles}</style>

not imported only — I’m curious to know the rationale behind this choice

It mainly has to do with figuring out whether the referenced identifier testStyles comes from the right scope. With imports it is easy since we can collect them into an array and as we encounter an identifier we can check whether it's been imported or not.

One cons of using external styles also is that since we don't know whether you will use them as global or scoped (or both) the resulting transpiled code contains both: twice as much code, probably fine when gzipped but with the current implementation it is not tree-shakable (not sure if tools like uglifyjs can delete it when doing dead code elimination).

coox commented 6 years ago

It was’t obvious for me when I started using styled-jsx that the build process you describe is pretty much the only way to achieve what I was trying to do… But thanks to this discussion, and with inspiration from Next.js’ with-global-stylesheet example, I managed to adapt your hypothetical build script as a webpack loader.

Now the following is possible, and what’s more, with hot module reload goodness!

import styleSheet from './page.css';

<style jsx global>{styleSheet}</style>

I have updated my demo repo accordingly.

The principle is to leverage Webpack (in my case, through next.config.js) to load CSS files as JavaScript modules exposing the style in a css-tagged template literal.

This involves a combination of:

As far as I’m concerned, the issue documents ways to address style injection from an outside file, and can certainly be closed.

I might want to publish this simple loader (edit: done!), this could prove useful! Also, I might want to submit this as a Next.js example.

Thanks a lot for your help and explanations, @giuseppeg.

giuseppeg commented 6 years ago

@coox I tried something like that once and yep it works for external styles. Although there are some cons:

One cons of using external styles also is that since we don't know whether you will use them as global or scoped (or both) the resulting transpiled code contains both: twice as much code, probably fine when gzipped but with the current implementation it is not tree-shakable (not sure if tools like uglifyjs can delete it when doing dead code elimination).

Those for me are kind of a big deal but it is subjective.

There is another option now that styled-jsx supports plugins. You could make a plugin that replaces comments with actual css:

<style jsx>{`/*@jsx-import(./page.css)*/`}</style>

https://github.com/zeit/styled-jsx/#css-preprocessing-via-plugins

To be honest this feels a bit like an hack, maybe you can use CSS Modules if you only care about external styles and want to use SASS and other tools :)

tom2strobl commented 6 years ago

thanks for making me aware of this thread @giuseppeg, just chiming in real quick that using the styled jsx scss plugin in @coox's setup worked flawlessly 👌🏻

We're mostly building visually intensive microsites which are less about data and more about entertaintment and our stylesheets tend to be humongous, so separating them into their own files is super important to us. Big thanks to both of you!

chibicode commented 6 years ago

Hi @giuseppeg, this is a slightly different scenario than @coox's situation, but kind of related and therefore I'd like your 👀 .

Background: Use inline-import babel plugin to import a CSS file into a string, and then stick it inside <style jsx global>

I added an example on next.js to load a CSS file located in node_modules: https://github.com/zeit/next.js/pull/3157

This uses inline-import babel plugin, which allows you to import a CSS file into a string, and is based off a comment from here: https://github.com/zeit/next.js/issues/544#issuecomment-325512576 - a good use case is to quickly import libraries like normalize.css or other CSS libraries, without having to do extra work outlined in the previous with-global-stylesheet example.

From the README on my PR

{
  "plugins": [
    [
      "inline-import",
      {
        "extensions": [".css"]
      }
    ]
  ],
  "presets": ["next/babel"],
  "ignore": []
}
import tachyons from 'tachyons/css/tachyons.min.css'
<style jsx global>{tachyons}</style>

Problem: It generates __jsx-undefined ID on style tag

But I'm not sure if this is considered as a hack and should be avoided. One reason I'm worried is that, if I run the above example, everything works perfectly except that the id of the generated style tag is __jsx-undefined:

<style id="__jsx-undefined">...</style>

Failed Attempts

Also, instead of

<style jsx global>{tachyons}</style>

If I do

<style jsx global>{`${tachyons}`}</style>`

It generates a non-undefined id on the style tag, but it generates extra curly braces at the beginning and at the end and therefore fails.

It'd be great if you could take a look. It's merged to master on next: https://github.com/zeit/next.js/tree/master/examples/with-global-stylesheet-simple

chibicode commented 6 years ago

I just realized that css template literal is responsible for generating the hash...but

css`${tachyons}`

Leads to extra brackets as mentioned by @coox, if the style is imported using global.

I could come up with a more elaborate build process as suggested on this thread, but that would defeat the purpose of keeping it simple...

dizlexik commented 6 years ago

I'm eagerly awaiting a resolution to this issue. This bug doesn't produce any symptoms for me in dev mode but when I build and run in a production environment I start getting errors with the message StyleSheetRegistry: styleId: 'jsx-undefined' not found.. For now I'm forced to pin my version of styled-jsx to v1 via yarn resolutions, but I'd really love to utilize some of the new styled-jsx features as soon as this issue is resolved.

giuseppeg commented 6 years ago

this is getting out of topic / styled-jsx and external styles weren’t meant to be used like this. If you want regular external css please consider using a custom solution maybe with a tiny custom react component to inject actual external css

giuseppeg commented 6 years ago

@coox feel free to open a new issue to enable support for css within the same component file. Also feel free to submit a PR for readme.md to describe how to use your setup

dizlexik commented 6 years ago

So the with-global-stylesheet-simple example in the Next.js repo is invalid then? And there's no plan on fixing the StyleSheetRegistry: styleId: 'jsx-undefined' not found. error?

This is a shame. It seems to me to be a valid use case and is exactly what I'm looking to accomplish. But if you're saying that it is beyond the scope of styled-jsx/css to accept a string constant (the value of which is known at build time) then I think at least it should throw a meaningful error message instead of working in dev mode and breaking mysteriously in production. It took me quite a while to track down what was causing this and to eventually find this GitHub issue.

Also, why does this work fine for me with styled-jsx v1 but not v2?

chibicode commented 6 years ago

@dizlexik sorry about causing a trouble in production, I didn't realize this method failed in production on v2 when I submitted that example. I probably should submit a PR to remove the example.

feel free to open a new issue to enable support for css within the same component file.

I think this is supported then it might work out though.

Sorry for getting off topic.

dizlexik commented 6 years ago

I actually implemented this independently before ever seeing your example and stumbled upon this only after running into the same issues as you did. Which probably means we're not the only ones that have been or will be burned by this.

dizlexik commented 6 years ago

So what is the recommendation for this? Currently I'm using yarn and have styled-jsx pinned at version 1.0.11 which supports this use case, but I'd like to update to the latest version of styled-jsx. I'm just not exactly sure what the best way to work around this is. Thoughts?

giuseppeg commented 6 years ago

@dizlexik if you are using Next.js you can use the Head component https://github.com/zeit/next.js/#populating-head to reference tachyons.css in a <link> tag (use a key also). If not you may want to implement a similar component (look at Head's implementation).

giuseppeg commented 6 years ago

In general if you want external files and don't need scoped styles you should avoid using styled-jsx.

dizlexik commented 6 years ago

Thanks @giuseppeg. I made the switch to using Head with a link element. Unfortunately now I'm experiencing FOUC when navigating to the page. I have no idea how to prevent this and I'm having trouble finding any discussion of this.

Also the CSS file I'm including comes from a library sitting in node_modules. This was fine when I was using babel-plugin-inline-import but now I've had to copy this file to a location that is served staticly, which I'm not thrilled about because now I'll need to remember to update this anytime the library is updated. I spent some time struggling to figure out a way with babel/webpack config to automatically copy this file to a location that is served staticly but in the end had no luck and ended up with the manual copy/paste step.

This all feels so hacky that I think I'm going to revert back to styled-jsx v1.0.11 again for now. Still extremely bummed that this stopped working in v2. My code using v1 feels so clean and performs wonderfully.

giuseppeg commented 6 years ago

@dizlexik I just released 2.2.0 which might enable what you were doing in the past. You may want to try it out. Next.js hasn't been updated yet but you can try to install it manually.

dizlexik commented 6 years ago

I was really excited to try this but unfortunately the behavior seems to be identical to v2.1.3. The page renders correctly from the server but not when navigating to it from another page. And when I navigate away from the page while it's rendered incorrectly I get the error: Error: StyleSheetRegistry: styleId: 'jsx-undefined' not found.

giuseppeg commented 6 years ago

@chibicode the syntax for external files changed you have to rewrite imports like so:

import css from 'styled-jsx/css';
const identifierName = css`${content}`;

See @coox's plugin

giuseppeg commented 6 years ago

@dizlexik are you using Next.js? If so, how did you integrate the new version of styled-jsx? Can you share a sample (minimal) repo?

dizlexik commented 6 years ago

This code:

const test = css`.test { color: #123; }`;
const styles = '.test { color: #123; }';
const test2 = css`${styles}`;
console.log(test);
console.log(test2);

produces this result:

[".test{color:#123;}", __hash: "586814798", __scoped: Array(1), __scopedHash: "3623753071"]
["{.test { color: #123; };}", __hash: "3576237725", __scoped: Array(1), __scopedHash: "1080400668"]

They should be identical, right?

And yes, I'm using Next.js. I installed 2.2.0 with yarn add styled-jsx@2.2.0 and I updated my package.json with "reslutions": { "styled-jsx": "2.2.0" } to pin the version. I checked node_modules/styled-jsx to ensure that I am indeed using v2.2.0.

giuseppeg commented 6 years ago

ok cool, for now forget this

const test2 = css`${styles}

we should fix it at some point I guess. Have you tried @coox's plugin? https://github.com/coox/styled-jsx-css-loader

I will fix Next's with-global-stylesheet-simple example when I have time

dizlexik commented 6 years ago

Yeah I think if that curly brace issue was fixed then my current solution would work fine.

I have actually spent a good deal of time with @coox's loader and haven't been able to get it working yet. You can follow along here: https://github.com/coox/styled-jsx-css-loader/issues/1

I'd be happy to get his solution working, but much happier if I didn't need it at all because it adds a new dependency and a fair amount of complexity and configuration that I'd rather avoid if possible.

dizlexik commented 6 years ago

I finally upgraded my styled-jsx and implemented the following to load styles from dependencies:

import Styles from 'library/css/styles.min.css'
...
<Head><style dangerouslySetInnerHTML={{ __html: Styles }} /></Head>