webpack-contrib / css-loader

CSS Loader
MIT License
4.3k stars 602 forks source link

css loader chokes on valid CSS #43

Closed haf closed 9 years ago

haf commented 9 years ago

https://gist.github.com/haf/a632962b86409b8c51e3

ERROR in ./~/css-loader?root=..!./css/front.css
Module build failed: Error: Please check the validity of the CSS block starting from the line #414
    at throwError (/node_modules/css-loader/node_modules/csso/lib/gonzales.cssp.node.js:399:15)
    at getBlock (/node_modules/css-loader/node_modules/csso/lib/gonzales.cssp.node.js:755:18)

As you can see, there's no line 414.

sokra commented 9 years ago

yeah. We propably need to implement our own parser, which just grabs url(...) and @import ....

haf commented 9 years ago

Ok...? That would need to allow all chars in RFC3986, and the CSS2 spec 4.3.4 and import like these. So that's implemented by these regexes:

Test-data:

@import url("fineprint.css") print;
@import url("bluish.css") projection, tv;
@import 'custom.css';
@import url("chrome://communicator/skin/");
@import url(chrome://communicator/skin/);
@import "common.css" screen, projection;
@import url('landscape.css') screen and (orientation:landscape);
var uriNoQ = /url\((?<path>([a-zA-Z\.\/\-_0-9:~?\[\]@!$&*+,;=]|\\')*)\)/,
    uriSingleQ = /url\('(?<path>([a-zA-Z\.\/\-_0-9:~?\[\]@!$&\(\)*+,;="]|\\')*)'\)/,
    uriDoubleQ = /url\("?(?<path>([a-zA-Z\.\/\-_0-9:~?\[\]@!$&\(\)*+,;=']|\\")*)"?\)/

var importUriNoQ = /@import url\(?(?<path>([a-zA-Z\.\/\-_0-9:~?\[\]@!$&*+,;=])*)\)?/,
    importUriSingleQ = /@import (url\()?'(?<path>([a-zA-Z\.\/\-_0-9:~?\[\]@!$&\(\)*+,;="]|\\')*)'\)?/,
    importUriDoubleQ = /@import (url\()?"?(?<path>([a-zA-Z\.\/\-_0-9:~?\[\]@!$&\(\)*+,;=']|\\")*)"?\)?/

since JS doesn't have recursion in their regexps. You'll have to find the index = 1 match group from them, since I don't think JS supports named regex either. The first case disallows urls with ) and ( because you need a way to know the 'end' if your uri string. Possibly we could allow escaping ) but I can't find that in the spec.

This works with my sample file.

Can you throw such a loader together now, please, so I can publish my site? I would really owe you one.

mtscout6 commented 9 years ago

Do you all have thoughts on using this css parser? It only breaks out the code to an AST which you could then query for url from ast trees that are of type import.

$ node
> var css = require('css');
undefined
> var ast = css.parse('@import url("some.css");');
undefined
> console.log(ast);
{ type: 'stylesheet',
  stylesheet: { rules: [ [Object] ], parsingErrors: [] } }
undefined
> console.log(ast.stylesheet.rules[0]);
{ type: 'import',
  import: 'url("some.css")',
  position:
   { start: { line: 1, column: 1 },
     end: { line: 1, column: 25 },
     source: undefined } }
undefined
> console.log(ast.stylesheet.rules[0].import);
url("some.css")
haf commented 9 years ago

Seems like that parser has a bug in the parsing of URLs with ), that I mentioned in my regex: https://github.com/reworkcss/css/issues/67#issuecomment-83348372

sokra commented 9 years ago

Ok...? That would need to allow all chars in RFC3986, and the CSS2 spec 4.3.4 and import like these. So that's implemented by these regexes:

I would like to make it simpler and just allow everything except the end mark:

/url\s*\(\s*"([^"]*)\s*\)/ etc. We also need to check for comments and ignore them.

Here is a simple module which combines statemachine with regex for parsing. i. e. the html-loader uses it to parse the html: https://github.com/webpack/fastparse

cc @jhnns

mgmorcos commented 9 years ago

:+1:

chrisguerrero commented 9 years ago

Any word on this?

haf commented 9 years ago

For me this has meant I can't use webpack right now. I did some looking into how to write a regex parser, but there's just so much undocumented structure that I don't recognise, so I gave up. I haven't had any other contact with anyone from webpack.

chrisguerrero commented 9 years ago

@haf I just got around this by switching the filename to .scss and using the sass-loader

haf commented 9 years ago

Ok, I had problems with that loader as well; other problems, like race-conditions breaking 25 minute builds (latest problem) and problems finding a stable version in general, unfortunately. If you look inside that repo for issues started by me, you'll see how interested they are in helping out.

jhnns commented 9 years ago

Thx @sokra. I'm totally busy right now (because I'm relocating) and haven't found the time to investigate this :disappointed: