christianalfoni / webpack-bin

A webpack code sandbox
http://www.webpackbin.com
MIT License
710 stars 75 forks source link

SASS do not support a @import statement #144

Open mikeifomin opened 8 years ago

mikeifomin commented 8 years ago

See example: http://www.webpackbin.com/N1Az33oG-


Module build failed: File to import not found or unreadable: /api/sandbox/14639973780838080/a.scss
Parent style sheet: stdin
      in /api/sandbox/14639973780838080/lib.scss (line 1, column 1)

Why? That is because in sass-loader do not working properly with a memory-fs. Sass works with the node-sass that supports the @import statement via importer function, which return a filename or contents. In sass-loader the importer return the filename and then node-sass(via LibSass) is trying to read from a physical file system. Of course

So technically this is sass-loader bug but at the crossroads the memory-fs.

christianalfoni commented 8 years ago

Yeah, this is a limitation currently. Other libs also causes this constraint. Kinda sucks, but... hm... I suppose it would be possible to make sass-loader use the Webpack reference to the filesystem instead of hardcoded to fs? So whatever filesystem is put on webpack is what is being used by sass-loader?

christianalfoni commented 8 years ago

Thanks for the research btw :-)

mikeifomin commented 8 years ago

Im so interested in fixing this feature-bug that I think to write PR in a few days. But Im a new in an internal API of the Webpack so it can take match more time) Maybe there is some simple string to fix it?

christianalfoni commented 8 years ago

My initial thought here is to fix the issue in sass-loader, so instead of using require('fs') it would point to the compiler instance filesystem. As you can see here it is changed: https://github.com/christianalfoni/webpack-bin/blob/master/server/sessionBundler.js#L82.

So if sass-loader is able to reference the compiler it should use that filesystem. By default the compiler uses require('fs').

Would that make sense?

mikeifomin commented 8 years ago

The last 4 hours of research I'v understand some of Webpack internals. However problem is in the sass-loader only.

Node-sass has an importer api and a result callback take an object with:

file (String) - an alternate path for LibSass to use OR
contents (String) - the imported contents (for example, read from memory or the file system)

The sass-loader use the first one option (direct link to source):

     done({
           file: resolvedFilename.replace(matchCss, '')
     });

So the LibSass takes a value from file ( an absolute url of a file need to be imported) and load via C api (LibSass is C library).

The easiest way to fix it is to use contents with data loaded by loader module

     function readFileFromWebpackFileSystem(_path){
          // ???
     }
     done({
           contents: readFileFromWebpackFileSystem( resolvedFilename.replace(matchCss, '') )
     });
mikeifomin commented 8 years ago

How to get an access to a Webpack instance from a loader function?

christianalfoni commented 8 years ago

Ah, okay! Hm, I have not written any loaders for Webpack, but if you log out this inside the loader callback there might be some reference to the webpack instance there. Or I think an issue is needed to get some help on it

mikeifomin commented 8 years ago

Yeah! With this._compiler.inputFileSystem the sass-loader working perfect.)

christianalfoni commented 8 years ago

Ah, awesome, maybe you could give them a pull on that. It should really work like that :-)

mikeifomin commented 8 years ago

Of course! But the PR is not ready due to some refactoring of a main app I developing. My estimation of the pull is about 8 hours. I think more convenient is to pull directly to the sass-loader. So approve may take some time.

christianalfoni commented 8 years ago

Yeah, I think pull directly to sass-loader is the best. It might be pushed out as a patch version though, as it does not really change anything... but, hm, probably take some time yes ;-)

But really great that we figured this one out! 👍

mikeifomin commented 8 years ago

Now finally I've created a fork and commit changes there. To be honest it will be better to add some tests with the memory-fs. The only test I've made is to put this string in my project and run it :grimacing:

  "sass-loader": "git@github.com:mikeifomin/sass-loader.git#memory-fs",
christianalfoni commented 8 years ago

Heh, yeah, that would be really great... though quite a bit of work. But always good to get feedback on the suggestion first anyways, author will just require tests if needed :)