Ignore parsing errors, fix #131

[asvetliakov]

Fixes #131

stylelint throws any error not related to "CssSyntaxError"
https://github.com/stylelint/stylelint/blob/ded86c81d055b3916ef3242a390fd7dad00ca8a7/lib/standalone.js#L277

Changes:

1. Ignore any parsing errors from babylon
2. Throw CssSyntaxError for wrong interpolation tags (previously was just Error) so stylelint can eat & handle them

[coveralls]

Changes Unknown when pulling bd90011 on asvetliakov:fix-131 into ** on styled-components:master**.

[emilgoldsmith]

Hey @asvetliakov, thanks for the PR! This is a great thing to get fixed! Also thank you for the link to the CssSyntaxError line, that was helpful in understanding your PR quickly :).

From a quick look I had a few thoughts, one is that it seems pretty overkill making postcss a dependency that we ship just for the CssSyntaxError, though one would of course always also have stylelint installed at the same time that has it as a dependency, so I guess maybe it wouldn't make a big difference? I guess if we just made stylelint a PeerDepency we could also just assume the presence of postcss and still do the same require, right? Or maybe a require('stylelint/node_modules/postcss/lib/css-syntax-error).

The second one was whether it is really necessary to wrap our whole processor in a try catch, I guess our module is pretty light weight so wrapping it in a try catch probably wouldn't be that bad, but I was thinking we could probably narrow it down a bit further though, to for example just wrapping these lines here as that's where Babylon actually does it's parsing, right?

What are your thoughts on those two points?

[asvetliakov]

From a quick look I had a few thoughts, one is that it seems pretty overkill making postcss a dependency that we ship just for the CssSyntaxError, though one would of course always also have stylelint installed at the same time that has it as a dependency, so I guess maybe it wouldn't make a big difference? I guess if we just made stylelint a PeerDepency we could also just assume the presence of postcss and still do the same require, right? Or maybe a require('stylelint/node_modules/postcss/lib/css-syntax-error).

I'm wondering if declaring postcss as peerDependency will throw warning and require user to install it either in dependencies or devDependencies since you're not installing it explicitly, only stylelint. In any case the postcss will be flattened in the node_modules, resulting into single package. We can declare semver to * just to allow all versions from stylelint. What do you think?

The second one was whether it is really necessary to wrap our whole processor in a try catch, I guess our module is pretty light weight so wrapping it in a try catch probably wouldn't be that bad, but I was thinking we could probably narrow it down a bit further though, to for example just wrapping these lines here as that's where Babylon actually does it's parsing, right?

I believe it's more logically catch error in cosumer calling code rather than parser wrapper. It's also easier to just return "" here instead of { extractedCss: "", sourceMap: {} } if it will be handled in parser

[emilgoldsmith]

I believe it's more logically catch error in cosumer calling code rather than parser wrapper. It's also easier to just return "" here instead of { extractedCss: "", sourceMap: {} } if it will be handled in parser

Yeah I think you're right about this, that was probably me being too petty, let's definitely do it like that.

We can declare semver to * just to allow all versions from stylelint. What do you think?

Yeah I think that's actually a good idea as well if I understand you right, I just did a bit of further research into how npm flattens the dependency tree and according to this SO post they won't install an extra package as long as versions don't conflict. So the way I understood you was installing postcss as a dependency, but allowing any version of postcss am I right? Which would then flatten the node_modules no matter what.

Are there any versions of postcss that don't have the css-syntax-error though? Or where the directory structure in the package is different? Or where the API for the constructor of the error is different? Because that could break the whole package which would suck.

Hmm yeah, I feel like we have quite a few options here, because we should probably anyway make stylelint a PeerDependency, I actually don't know why we haven't yet, and if we do that postcss would always be installed. And then for our own dev I guess we could just add it to devDependencies? Or maybe figure out which versions of postcss have a consistent API for the error class and then allow all versions greater than that or something.

Or a final option could be to just copy paste the code from the postcss library to our repo, which I feel like is probably no more vulnerable as any of the other options, probably less, and we would also have more control of what code exactly is being run in our repo, so I think I lean towards this.

I would greatly appreciate more thoughts on this from @asvetliakov, @mxstbr and @ismay though as I don't think it's super clear which way to go or if there's a superior way to all that I already suggested.

[ghost]

I personally wouldn't worry too much about having postcss as a dependency. Why not just add the correct version of postcss as a dependency? It might make our package bigger, but I'm not too bothered by that. Primary concern would be that it works correctly and smoothly in my opinion.

[emilgoldsmith]

It might make our package bigger, but I'm not too bothered by that. Primary concern would be that it works correctly and smoothly in my opinion.

Hmm now that I think about it, I guess our package is also only run on the server + only run in dev, so it's not like it would ever cause bloating of anything ever sent over the internet, so maybe you're right and we should just be explicit.

[nemisj]

@emilgoldsmith do you think this PR can be merged in? It would be cool to have it, since it works nicely with create-react-app

[emilgoldsmith]

Yeah I think all my issues have been resolved, and it doesn't seem like anyone else has any problems with it, so let's merge it in :).

I'm not sure exactly what you mean @nemisj about this PR making it better for CRA though, but sure, it's of course an upgrade as always :). I'll merge it in and publish a minor version as this shouldn't be breaking.

[nemisj]

@emilgoldsmith i will explain, if you want :)

In CRA they use babel-loader, which have this nize extended error snippet if you do some error in your code, with lines surrounding, etc. So you can easily see where it breaks.

Because I use now styled-components linter as a loader in the webpack configuration too, it get's executed before babel-loader, so instead of having nice error with SyntaxErorr stuff, i get only a little line with name of the file and line number

With this change linter won't break on SyntaxErrors of babylon, and webpack will proceed with babel-loader, which will break showing me nice error.

[nemisj]

@asvetliakov thanks, works like charm 👍