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

Please provide a performance test #3

Closed
jhnns opened this issue Dec 29, 2016 · 6 comments
Closed

Please provide a performance test #3

jhnns opened this issue Dec 29, 2016 · 6 comments

Comments

@jhnns
Copy link

jhnns commented Dec 29, 2016

HI yibn2008

thanks for providing an alternative solution. I'm one of the maintainers of sass-loader and I think it's great to see where we can get better. Could you please provide a performance test that proves your performance numbers? I would like to see where sass-loader is slower than fast-sass-loader.

I would also like to understand why fast-sass-loader's cache is better than webpack's cacheable(). As far as I could tell by reading the code it's currently working exactly the same but maybe I've missed something.

@yibn2008
Copy link
Owner

yibn2008 commented Jan 5, 2017

@jhnns

I'll provide some test cases later.

When use ExtractTextPlugin.extract() with css-lodaer!sass-loader, the loaders will be invoked twice, and the second invoke is unnecessary, that's why I use internal cache to avoid the second compilation.

And the most important improvement is sass deduplication. With the increase of sass files, duplicate
sass files will seriously affect the compilation performance.

@jhnns
Copy link
Author

jhnns commented Jan 5, 2017

Both cases should be caught by webpack's cacheable flag, though it could be a bug/unintended behavior of the ExtractTextPlugin to circumvent the cache.

@yibn2008
Copy link
Owner

yibn2008 commented Jan 5, 2017

cacheable cannot solve the problem of sass duplication, and due to the bug of ExtractTextPlugin, this flag doesn't really work.

@yibn2008
Copy link
Owner

@jhnns performance test case has been provided, please see https://github.com/yibn2008/fast-sass-loader#performance

@jhnns
Copy link
Author

jhnns commented Mar 21, 2017

Awesome. Your test case might give us a clue why webpack's resolver is taking so long 👍

A note on sass deduplication: This has been suggested several times, but I think that it's not correct to implement this in the sass-loader since it would produce a different output than node-sass. You don't have any confidence about the order of the generated CSS.

I think it's better to divide your sass files into application files and library files. Application files produce actual CSS and should only be imported once. Library files don't produce any CSS, but provide variables and mixins. Thus you can import them without duplicating CSS.

@yibn2008
Copy link
Owner

@jhnns Yes, the output would be different when you have same variable in different sass file, but It can be avoid by using variable prefix.

In large sass project, manage sass library without duplicating css is really a hard work, the project members need to be very careful with these dependencies, but using variable prefix will be very easy, that's why we use sass dedupe in fast-sass-loader.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants