thgh / rollup-plugin-scss

Rollup and compile multiple .scss, .sass and .css imports
MIT License
134 stars 47 forks source link

added an option to choose the sass compiler #40

Closed riri closed 4 years ago

riri commented 4 years ago

Fixes issue #39

Using node-sass by default, as wanted by the author.

Just adding a runtime option to the plugin to set a compiler object replacing node-sass.

import scss from 'rollup-plugin-scss'
import sass from 'sass'

export default {
  input: 'input.js',
  output: {
    file: 'output.js',
    format: 'esm'
  },
  plugins: [
    scss({
      runtime: sass
    })
  ]
}
Pk13055 commented 4 years ago

Can you check if this fixes #41?

riri commented 4 years ago

Can you check if this fixes #41?

Clone my forked repository and use it in your package.json to test against the evolution

thgh commented 4 years ago

It looks like yarn run test won't pass, is it?

riri commented 4 years ago

It looks like yarn run test won't pass, is it?

I haven't touched the test script, but added a test-sass, and tests pass on my system I think the tests need to be improved to not have end-of-line system dependencies.

greym0uth commented 4 years ago

Thanks for adding this feature! :D @thgh Could you take a look at this now that the tests are passing?

greym0uth commented 4 years ago

Oh I almost forgot, since this package uses yarn could you remove the package-lock.json and run yarn install to regenerate the yarn.lock.

riri commented 4 years ago

I don't have much time lately to check how to implement correctly all change requests. I'll code them as soon as possible. Just to summarize: 1) yarn or npm, that's a matter of choice for most developers, it's a mess. I'll switch back to yarn.lock to go to the author preference. I just didn't think of it and use npm myself 2) I'll keep node-sass as a full dependency, even if not used by the importing project. I don't like this solution (because node-sass needs more than sass), but I don't see how to fix it without changing the default implementation). On the other hand, users should be aware that peer dependencies are required (it's printed during install) 3) ok for changing the option name to compiler :)

thgh commented 4 years ago

Hi @riri, do you mind checking out #44? If the merge looks OK to you, I will merge it!

@furstenberg & @niklasbuschmann, your input is also very welcome!

furstenberg commented 4 years ago

I’ve been using it for several weeks already. For my use it works great.

Thank you for the plugin and @riri for this pull request.