Skip to content
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

css loader chokes on valid CSS #43

Closed
haf opened this issue Apr 2, 2015 · 11 comments
Closed

css loader chokes on valid CSS #43

haf opened this issue Apr 2, 2015 · 11 comments

Comments

@haf
Copy link

haf commented Apr 2, 2015

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
Copy link
Member

sokra commented Apr 2, 2015

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

@sokra sokra added the type: Bug label Apr 2, 2015
@haf
Copy link
Author

haf commented Apr 2, 2015

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
Copy link

mtscout6 commented Apr 2, 2015

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
Copy link
Author

haf commented Apr 3, 2015

Seems like that parser has a bug in the parsing of URLs with ), that I mentioned in my regex: reworkcss/css#67 (comment)

@sokra
Copy link
Member

sokra commented Apr 3, 2015

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
Copy link

mgmorcos commented Apr 6, 2015

👍

@chrisguerrero
Copy link

Any word on this?

@haf
Copy link
Author

haf commented Apr 7, 2015

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
Copy link

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

@haf
Copy link
Author

haf commented Apr 7, 2015

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.

@sokra sokra closed this as completed in 3109c97 Apr 9, 2015
@jhnns
Copy link
Member

jhnns commented Apr 28, 2015

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

@sokra sokra mentioned this issue May 14, 2015
koistya pushed a commit to koistya/css-loader that referenced this issue Nov 19, 2015
Apply default singleton to IE6-8 as well
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants