-
-
Notifications
You must be signed in to change notification settings - Fork 444
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
Support new SASS processors #271
Conversation
@@ -7,7 +7,15 @@ | |||
begin | |||
require 'sassc-rails' | |||
rescue LoadError | |||
raise LoadError.new("bootstrap-rubygem requires a Sass engine. Please add dartsass-sprockets or sassc-rails to your dependencies.") | |||
begin |
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.
I hate the way these rescues nest, but I can't think of a better way for chaining requires like this.
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.
#272 suggestion
I don't think checking for these gems in the engine is necessary/appropriate at all. Instead, I'd propose to remove these checks completely and let the user decide how to do their bundling. 🤷 |
@manuelmeurer I agree. Although I didn't want to make major changes to avoid a potentially lengthy discussion - my priority was to unblock using this gem with the new SASS processors. We can follow up discussing removing the requires altogether after this is merged. |
Ok, let's wait for a maintainer to chime in! 😄 |
Removing the requires would be good but we'd need to make sure it doesn't break backwards compatibility too badly. |
I don't think backwards compatibility would be an issue here, since all these requires do is, well, requiring the gems. 😄 |
@manuelmeurer Yes please. Let's also have a few people test the PR. One thing that might go wrong is the require order -- I don't know if any of these preprocessors must be required before bootstrap. |
@glebm @manuelmeurer Hi, there seems to be something else that needs to be fixed. Please look at this ticket: #277 |
I didn't notice it at first, because macos has by default: JavaScriptCore and it is used, but docker doesn't have it and the error is immediately noticeable there |
I'll backport this to branch 4.6-stable unless someone is already working on it |
Summary
The gem currently only supports current-gem SASS processors -
dartsass-sprockets
andsassc-rails
. The PR adds support fordartsass-rails
andcssbundling-rails
.Closes #247