Rewrite parseStyle parser to correctly transform parenthesis and quotes

[lfittl]

Previously the parser would simply split by ;, which is not correct
for CSS like background-image: url('data:image/gif;base64,...').

Instead of using a split string approach, write the parser as a simple
character-by-character loop that keeps state as it walks the CSS string.

In case additional edge cases are discovered in the future that keeps
the parser easily extendable, and more readable (and likely faster) than
a complex regular expression.

[wooorm]

Hey Lukas! Thanks for the PR!

This is similar to GH-12, and my comments there apply to this as well. What do you think after reading that?

[lfittl]

@wooorm Thanks, I didn't see that PR earlier.

I think the problem with that stance (i.e. keeping this library simple and suggesting to use object arguments) is that it doesn't allow using this library in a more complex environment where you don't control all inputs directly.

Case in point, the reason I have run across this is because of wanting to use React components inside Gatsby Markdown files. The approach described indirectly relies on this library (something that actually took me some time to figure out, not being familiar with this ecosystem), through https://github.com/rhysd/rehype-react.

The actual trigger for the bug however is here, in a totally different package (the one creating the style attribute): https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-remark-images/src/index.js#L100

You can see from gatsbyjs/gatsby#3545 that this error is very hard to debug, and will likely lead to users chasing the wrong direction for multiple hours.

So in summary, I think this library needs to do at least the basics of CSS parsing right.

My patch is self-contained enough and shouldn't cause any performance issues, so besides the slightly higher complexity I think this is the best way forward. If one e.g. wanted to add support for comments in the future that would be fairly easy to add with a few lines of code.

[lfittl]

@wooorm Just as one more data point, seems like one more person ran into this on the gatsby issue caused by this: gatsbyjs/gatsby#3545 (comment)

What do you think - is fixing this really out of scope for this library?

[wooorm]

Sorry for the late response!

I’d really like to keep this small. If we add a good solution (css-declarations) that would add ± 133kb unminified, 43kb minified, and that’s a lot. That’s fine in the gatsby world, but huge in comparison to the current size of this lib.

Now, as this utility does not process style as an object, we could just as well write a little utility that processes style from string to object: Let’s call it hast-util-parse-css.

var parse = require('css-declarations').parse
var visit = require('unist-util-visit')

module.export = transform

function transform(tree) {
  visit(tree, 'element', visitor)
}

function visitor(node) {
  var props = node.properties
  if (typeof props.style === 'string') {
    props.style = parse(props.style)
  }
}

I’d imagine putting this up in the readme, and linking to it, so people know to use it?

[lfittl]

@wooorm It still seems dangerous to me that people would run into this incomplete parsing behavior by accident and not even know they need to look at this library for the fault.

That said, I understand your argument, and it sounds like you're set on not adding this functionality to the library. Adding a warning to the README that urges people to use objects instead of strings (via something like your suggested code snippet) seems a fair solution thats definitely an improvement over the status quo.

[pgray-argo]

@lfittl Did you find a solution within Gatsby that solves this issue? I am encountering exactly the same problem that you referenced in your comment on April 1st (using custom React components in Markdown in Gatsby, gatsby-remark-images not carrying style correctly).

I'm not sure how to implement the code snippet that @wooorm suggested in his comment within Gatsby and could use some guidance.

As an aside, I'd like to also throw my vote behind landing this PR and making the library handle all parsing cases. 43 kb seems like an incredibly small price to pay for fully complete parsing behavior. This was an incredibly difficult issue to track down, and to find out that there is a fix but it's not going to be implemented is pretty frustrating.

[lfittl]

@pgray-argo I haven't had time to look into any higher-level fix inside Gatsby (i.e. switch to another library), and probably won't have time in the near future.

For now my workaround is to use my personal branch of this library (yarn add git://github.com/lfittl/hast-to-hyperscript).

[mjmeintjes]

Thanks. Just spent a couple of hours diagnosing this issue. Was just about to make a PR when I saw this one.

[TuckerWhitehouse]

@wooorm would you consider landing a PR that uses something like style-to-object? It uses the same parser as css-declarations, but weighs in at only 4.8 kB minified.

[mjmeintjes]

Another option would be to just throw an exception when the style attributes contains something like ;base64, so that at least it is obvious when this happens. Then people can search for this issue and use the fixed version.