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

Dart sass #19

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

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.

2 participants