-
-
Notifications
You must be signed in to change notification settings - Fork 33
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
upgrade to latest sass #32
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this require a new version release? Also, do you want to be added as a maintainer for the addon?
Yes, this would require a new version. Sure, you can add me then. |
@@ -27,18 +27,18 @@ module.exports = function(sass) { | |||
this.inputFile = inputFile; | |||
this.outputFile = outputFile; | |||
|
|||
this.renderSass = rsvp.denodeify(sass.render); | |||
this.renderSass = sass.compileAsync; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain inline what each of these changes does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now legacy code: https://sass-lang.com/documentation/js-api/modules#render
The new way is with
https://sass-lang.com/documentation/js-api/modules#compileAsync
index.js
Outdated
@@ -55,7 +55,7 @@ module.exports = function(sass) { | |||
throw new Error('[string exception] ' + error); | |||
} else { | |||
error.type = 'Sass Syntax Error'; | |||
error.message = error.formatted; | |||
error.message = error.message; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message now has the formatted text
This is a breaking change because the peer dep of sass needs to be a a new version? If so, should that be documented? |
Yes, it needs a newer sass version. Or continue to also support older version |
I changed it so that older versions are also supported |
@patricklx what do you think about doing a PR implementing the release process described in https://github.com/adopted-ember-addons/program-guidelines#release-process |
I can do it. But it will be next week |
to setup the release process I would use https://github.com/rwjblue/create-rwjblue-release-it-setup which needs access to this repo. So, either you do it or add me as maintainer |
any update on this? I would also be happy to be a maintainer and do it myself then |
Invite sent. |
fixes #31 #30