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

Use sass stats for marking dependencies instead. #29

Closed
wants to merge 1 commit into from
Closed

Use sass stats for marking dependencies instead. #29

wants to merge 1 commit into from

Conversation

riovv
Copy link

@riovv riovv commented Nov 6, 2014

I think we should be using the stats provided by sass compiler in order to mark the dependencies.
It contains all includedFiles which is exactly what webpack needs to know about.

Pro's:
Cleaner code
Removed one external dependency (sass-graph)
Better performance
Windows compatibility (sass-graph has issues regarding this)

Con's:
None that I've found. Please do tell if I missed something crucial.

@waldreiter
Copy link

@akiran had explained the reason for sass-graph in #17.

The biggest problem with sass-graph is that it supports only a subset of Sass. Also it throws errors on perfectly valid Sass input. I had to roll back to sass-loader 0.2.0. So I am in favour of this pull request.

@akiran
Copy link
Contributor

akiran commented Nov 7, 2014

webpack-dev-server won't work if we mark dependencies only in success handler.

If sass-graph has problems, we have to find a better solution to mark dependencies in both success and error cases.

@akiran
Copy link
Contributor

akiran commented Nov 7, 2014

I tried to debug this issue and found that value of stats.includedFiles is incorrect with node-sass version 1.2.2.
One of the dependencies is missing and additional 'stdin' is found in stats.includedFiles
Filed an issue: sass/node-sass#522

@waldreiter
Copy link

In #32 @appsforartists reports problems with sass-graph too.

The real fix will probably take some time. For now maybe sass-loader could get an option for sass-graph? Then those who want to have the watch functionality can easily activate it.

@akiran
Copy link
Contributor

akiran commented Nov 16, 2014

@Patrio Issues with sass-graph are fixed in #35
As you mentioned, sass-graph dependency will be removed after this feature sass/libsass#638 is supported in node-sass

@xzyfer
Copy link

xzyfer commented May 5, 2015

FTR I'm perfectly happy to fix any issues with sass-graph if they're reported. We use sass-graph in node-sass so we're committed to making sure it works for everyone.

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

Successfully merging this pull request may close these issues.

4 participants