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

Default Sass implementation to Dart Sass #192

Conversation

jbailey4
Copy link
Contributor

@jbailey4 jbailey4 commented Sep 4, 2018

Recently, in PR #186 support for multiple sass implementations (e.g. Dart Sass, LibSass, etc.) was added. This PR defaults the Sass implementation to Dart Sass, while adding instructions for those who want to use alternatives.

Now users can simply run: ember install ember-cli-sass

@jbailey4 jbailey4 force-pushed the auto-install-sass-implementation branch 2 times, most recently from 2e7f805 to 2ad8468 Compare September 4, 2018 03:05
@jrjohnson
Copy link

Oh good. I was just working on this myself! Glad you beat me to it.

I was exactly at this point when I had a thought: maybe we should fully default to the JS dart sass implementation and instead of putting it into the app with addPackageToProject it should just be in the dependencies section of package.json. This would provide it by default to apps, but It would still be possible to install a different implementation if you want to and I don't think the extra NPM install will bother anyone as it wont add any weight to the final output.

I don't think it matters too much either way, but it feels to me like the sass implementation is a detail that I (as an app author) would rather hand over to this addon instead of needing to manage it myself.

@simonexmachina
Copy link
Collaborator

Thanks @jbailey4! Seems to be a build error...

@jrjohnson I think adding the SASS implementation as a devDependency is the right approach.

@jbailey4
Copy link
Contributor Author

jbailey4 commented Sep 4, 2018

Seems to be a build error...

@aexmachina I saw this same syntax error locally and thought I pushed up the change. May need to restart the CI build

@jbailey4 jbailey4 force-pushed the auto-install-sass-implementation branch 2 times, most recently from 078cef4 to 9d5b82e Compare September 4, 2018 22:53
@jbailey4 jbailey4 force-pushed the auto-install-sass-implementation branch from 9d5b82e to 0f1c411 Compare September 5, 2018 00:46
@jbailey4
Copy link
Contributor Author

jbailey4 commented Sep 5, 2018

@aexmachina Tests look good now - had one node eslint issue.

Copy link
Collaborator

@simonexmachina simonexmachina left a comment

Choose a reason for hiding this comment

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

Nice!

@simonexmachina simonexmachina merged commit ee17f6b into adopted-ember-addons:master Sep 5, 2018
@simonexmachina
Copy link
Collaborator

Published v8.0.1

@jbailey4 jbailey4 deleted the auto-install-sass-implementation branch September 5, 2018 15:31
@javve
Copy link

javve commented May 8, 2019

I just want to say that Dart Sass made the SCSS re-compilation 6x slower in our app with ~140 .scss files (which basically means that the overall re-compilation got 5x slower).

This project really needs a changelog so people don't miss these "breaking" changes 😅

@simonexmachina
Copy link
Collaborator

Good suggestion @javve. See #208.

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.

4 participants