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

Add support for other SASS implementations #63

Merged
merged 9 commits into from
Feb 23, 2021

Conversation

ngryman
Copy link
Contributor

@ngryman ngryman commented Jan 8, 2020

Following up on the recent discussions in #34, this PR brings support for any SASS-compatible library.
As a direct consequence dart-sass can be used as a drop-in replacement for node-sass.

New implementation option

By default fast-sass-loader will try to require node-sass like before. If you want to override that behavior, you can use the new implementation to pass a different SASS implementation:

{
  loader: require('fast-sass-loader'),
  options: {
    implementation: require('sass')
  }
}

Note that I've followed the exact same API that sass-loader exposes to ease migration from sass-loader to fast-sass-loader.

New sassOptions option

If you need to pass additional options to the underlying implementation, it can now be done via the sassOptions option:

{
  loader: require('fast-sass-loader'),
  options: {
    implementation: require('sass'),
    sassOptions: {
      fiber: require('fibers')
    }
  }
}

Same as the previous I've followed the same API as sass-loader.

Caveat

There is one minor caveat that comes with this new feature, we would need to deprecate the outputStyle option in favor of sassOptions.outputStyle. The main reason is that dart-sass and node-sass are not exactly compatible here, and accept a different set of values. nested, which was the default value, is not support by dart-sass.

I think this change is beneficial maintainance-wise. Nesting SASS options in sassOptions and passing it directly to the render function gives access to all available SASS options out of the box, without having to cherry pick and map these options in the loader options.

@yibn2008 Let me know if this change is fine with you. If so I suppose you could publish a major version of this loader once this PR gets merged, and mention that people should switch from outputStyle to sassOptions.outputStyle.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.5%) to 85.372% when pulling 1aabc31 on ngryman:feat/support-other-sass-impl into 3329f0b on yibn2008:master.

@xmsz
Copy link

xmsz commented Feb 24, 2020

When to publich

outputStyle: 'nested',
resolveURLs: true
resolveURLs: true,
sassOptions: {}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to avoid break change, you should add compatible code for outputStyle , make sure the old way can still work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've explained why I needed to make that change in the "Caveat" section of the PR's description. Would it be an issue for you to publish a major version of this package after this PR is merged?

outputStyle: 'compressed'
sassOptions: {
outputStyle: 'compressed'
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not change test case of old options API, you should add a new case for sassOptions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cf. comment above.

@Alex-Hobbs
Copy link

Any updates on when this PR will be accepted/released? Looking forward to switching from sass-loader to fast-sass-loader but need the Dart support

@ngryman
Copy link
Contributor Author

ngryman commented May 8, 2020

fwiw I ended up publishing a new package here: https://www.npmjs.com/package/awesome-sass-loader.

@ngryman ngryman closed this Jul 11, 2020
}
const options = utils.mergeDeep(defaults, loaderUtils.getOptions(ctx) || {})
const includePaths = options.includePaths
const basedir = ctx.rootContext || options.context || ctx.options.context || process.cwd()
const transformers = createTransformersMap(options.transformers)
const implementation = options.implementation || require('sass')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI – this doesn't work. I believe it fails to deep copy the reference to sass or node-sass from options.implementation

@alexhobbs
Copy link

@yibn2008 if you're not going to maintain this package can you please allow others to take over? This needs to be merged in.

@yibn2008 yibn2008 reopened this Feb 23, 2021
@yibn2008 yibn2008 merged commit b09fba5 into yibn2008:master Feb 23, 2021
@yibn2008
Copy link
Owner

@alexhobbs sorry for taking long time to reply, I've been very busy with my work recently, this PR has been merged in.

@yibn2008
Copy link
Owner

@ngryman thanks for your great work 👍

@yibn2008
Copy link
Owner

  • fast-sass-loader@2.0.0

@alexhobbs
Copy link

@yibn2008 amazing, thank you very much!

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.

7 participants