Skip to content

Dart sass #19

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Dart sass #19

wants to merge 4 commits into from

Conversation

localnerve
Copy link

A 1.0.0 proposal

  • Uses dart-sass by default
  • Allows node-sass to be used instead via new sass option
  • Node 10+
  • Github Action Workflow

@koenpunt
Copy link
Contributor

I appreciate the effort, but there's really too much going in this PR; formatting, code style, more modern javascript language constructs, etc.

The code formatting for example is probably based on some auto formatting, but there doesn't seem to be a configuration for it. If any I would prefer Prettier to be used for formatting.

But it has been a long time since I worked on this project, and going through the code myself I see quite some things I would've done differently now. But I'm not using it anywhere anymore, so I don't really have bandwidth to work on it. And large PRs like this one makes it hard to review in a timely fashion..

If you're okay with maybe splitting it into several PRs, for example first updating the code style (with a file dictating or enforcing the style), and then one with the actual changes it would make it much easier for me to review.

@localnerve
Copy link
Author

I appreciate your thoughts on this.
Thanks for making this library. It was helpful to me and probably many others for many years.
The last thing I want to do is cause you any grief.

I submitted this PR to 'pay it back', and reflects work I did to keep this functionality alive in my own sass toolchains. It's required, bc the types returned by node-sass are not compatible with current sass (where all the feature work is being done).

My work is publicly available in NPM as package @localnerve/sass-asset-functions. Uses the current sass, but allows backward compatibility to node-sass for older projects.

However, as this package still gets ~5k downloads a week, I thought it could be beneficial for people by extending that upgrade path within this library. Perhaps a new package is the best path forward.

Thanks again for creating this library.

@koenpunt
Copy link
Contributor

I've released a new version to npm that replaces node-sass with sass (Dart Sass). Development will continue in this fork: https://github.com/koenpunt/node-sass-asset-functions

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants