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

Allow specifying Dart Sass implementation #9629

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

christopher-francisco
Copy link

@christopher-francisco christopher-francisco commented Sep 11, 2020

Description

For cases where you're not able to completely remove node-sass, causing sass-loader to pick it up, you can now explicitly specify you want to use Dart Sass through USE_DART_SASS=true env variable. See #9628 for the complete use case

Testing strategy

Manually tested it by doing the following

  1. Created a new app with yarn create-react-app my-app
  2. Added a few sass files with @use directive (only supported by Dart Sass)
  3. Added sass dependnecy
  4. Ran yarn start. Comiplation succeeded
  5. Added node-sass dependency (we now have both)
  6. Ran yarn start. Compilation failed
  7. Ran USE_DART_SASS=true yarn start. Comiplation succeeded

Screenshot

Screen Shot 2020-09-11 at 11 57 56 AM

You can see both sass and node-sass installed at the bottom right corner. App started with USE_DART_SASS=true yarn start

What's Missing?

  • Find a way to automate a test similar to the one described above
  • Updating documentation

Closes #9629

For cases where you're not able to completely remove `node-sass`,
causing `sass-loader` to pick it up, you can now explicitly specify you
want to use Dart Sass through `USE_DART_SASS=true` env variable.
@facebook-github-bot
Copy link

Hi @christopher-francisco!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@amyrlam amyrlam removed their request for review September 12, 2020 02:39
@just-boris
Copy link
Contributor

Is there a chance to make it default in version 4.0 which is coming soon?

Dart-sass (or just sass, since it is transpiled to pure JS anyway) supports more features and should be used whenever you need sass, except some exotic use-cases.

@stale
Copy link

stale bot commented Dec 25, 2020

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Dec 25, 2020
@just-boris
Copy link
Contributor

node-sass is deprecated, so this feature needs to be added, to allow consumers specifying the supported version of sass

@stale stale bot removed the stale label Dec 25, 2020
@stale
Copy link

stale bot commented Jan 9, 2022

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

@stale stale bot added the stale label Jan 9, 2022
@just-boris
Copy link
Contributor

This is not stale

@stale stale bot removed the stale label Jan 9, 2022
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.

3 participants