-
Notifications
You must be signed in to change notification settings - Fork 11
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
Make Sass implementation agnostic ( supporting dart-sass ) #17
base: master
Are you sure you want to change the base?
Conversation
}) | ||
}) | ||
}) | ||
describe.each(['node-sass', 'sass'])('with require("%s")', function(impl) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently installed version of jest did not support describe.each
, so I updated jest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for the record, the test failure was not due to the jest version ( it also failed with [email protected]
with ['node-sass', 'sass'].forEach
instead of describe.each
)
Love this, can we get it merged @koenpunt ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see I still have a pending review comment here.
options = options || {} | ||
options.images_path = __dirname + '/images' | ||
options.fonts_path = __dirname + '/fonts' | ||
options.implemantation = sass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options.implemantation = sass | |
options.implementation = sass |
hey folks, can this PR get merged in? would be super helpful since node-sass is basically decrepit at this point |
for those who want to install this now
|
I've released a new version to npm that replaces |
Resolves: #16
I made this library to be sass-implementation agnostic, that means, add support for
dart-sass
.With these changes, node-sass-asset-functions now accepts
implementation
option ( which exceptsrequire('node-sass')
orrequire('sass')
, and when absent it will be implicitlynode-sass
)For some reason the test is not passing ( Maybe
dart-sass
cannot read file correctly, even though the path looks correct ), which is now I'm searching why... Help wanted by someone.