-
-
Notifications
You must be signed in to change notification settings - Fork 446
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
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