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.

Sign up for GitHub

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

Jump to bottom

Supporting option for dart-sass ? ( or drop node-sass as dependency ) #16

Open
fsubal opened this issue Sep 12, 2018 · 3 comments · May be fixed by #17
Open

Supporting option for dart-sass ? ( or drop node-sass as dependency ) #16

fsubal opened this issue Sep 12, 2018 · 3 comments · May be fixed by #17

Comments

@fsubal
Copy link

fsubal commented Sep 12, 2018 •
edited
Loading

Maybe it could be solved by making a fork (since this package is **node-sass-**asset-functions )

Some people are now using dart-sass (or sass ) instead of node-sass, because of the painfulness caused by the native extension :(

Like sass-loader made itself sass-implementation-agnostic, it would be great to make node-sass-asset-functions also implementation-agnostic.

I found that node-sass-asset-functions itself runs well with sass, but even when I don't need node-sass as dependency, install node-sass-asset-functions forces my machine to rebuild node-sass.

2018-09-12 23 35 19

How it should be like?

Taking sass module as a parameter

assetFunctions({
    ....
    implementation: require('node-sass') // or require('sass')
})

// or even...
assetFunctionsFor(require('node-sass'))({
    ....
})

Then drop node-sass from dependencies, and it allows users to avoid forced rebuilding node-sass.
@koenpunt
Copy link
Contributor

This maybe can be achieved by making node-sass an optional dependency.

@fsubal
Copy link
Author

fsubal commented Sep 15, 2018 •
edited
Loading

Hmm.. as far as I examined, optionalDependency seems like a way of "avoiding installation failure" ( like chokidar allows non-Mac user skip installing fsevents ), not something to skip installing even if it's possible.

In this case, I think, node-sass-asset-functions should not install both of sass and node-sass, which means sass and node-sass only appears in unit test as devDependencies ( every user must install explicitly sass or node-sass by themselves ). This is what sass-loader does.

One thing to concern is this would be a breaking change, for those who only installed node-sass-asset-functions without installing their own node-sass

@fsubal
Copy link
Author

fsubal commented Sep 15, 2018 •
edited
Loading

Maybe optional dependency is better, if you meant, that it should only skip installing when failed but auto-installation-via-dependency itself is required ( and want to avoid breaking change )

@fsubal fsubal linked a pull request Oct 8, 2018 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make Sass implementation agnostic ( supporting dart-sass ) fsubal/node-sass-asset-functions
2 participants
@koenpunt @fsubal