-
Notifications
You must be signed in to change notification settings - Fork 437
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
Require sassc only in development #520
Conversation
@RobinDaugherty is this something you can confirm is working? |
module BetterErrors | ||
# @private | ||
module ErrorPageStyle | ||
def self.compiled_css(for_deployment = false) | ||
require "sassc" unless defined? SassC |
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.
The condition here isn't strictly necessary, since multiple calls to require
will short-circuit on second-and-subsequent invocations.
require "sassc" unless defined? SassC | |
require "sassc" |
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.
Good point — I was being overly cautious here given my inability to get passing tests locally. I've updated this to remove the conditional.
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 looks better to me, though it's important to note that I'm just another user of this project. 😄
Sorry about the delay here. All of the maintainers (including me) seem to be busy with other projects. Sorry that I introduced this bug...release 2.10 apparently didn't get enough feedback before release, and it was blocking other changes that needed to get out. I believe this PR is the correct way to fix #516, but I would really like to hear from someone that can verify it works in their project that does not include |
Hi, thanks for you attempt to fix this. Actually this allows to start the application, but (of course) doesn't work when it is time to render an error I can provide a reproducible test case if needed. Of course if I add sassc as a development dependency this will work, but sassc is usually not something that you want to add back because of its compile time and native extensions My suggestion would still be to pre-compile the css and distribute it with the gem, or allow some sort of customization to serve the asset from the application's pipeline. Also releasing a node package with the scss would help (see rails admin) I can provide some help by editing the |
I was able to verify that this works, thanks for the help on this! |
@RobinDaugherty apologies for my previous message, I confirm that this approach works, thanks! |
I have just updated this gem in our application and can confirm I can see errors correctly. Thank you for your work. |
The gem was added because the 2.10.1 release of better_errors broke the build. A new release 2.10.1 fixed the issue and SASSC is no longer needed in the application. The css is compiled and included in the built gem. Breaking Change: BetterErrors/better_errors#498 Fix: BetterErrors/better_errors#520
This change moves the
require
call to only be invoked the first time that the Sass files are compiled. This solves the issue of applications that don't use Sass loadingbetter_errors
without needing to installsassc
(see #516), and is an alternative resolution to #515.Note: Some specs are failing for me that seem unrelated to this change, and are present even when running specs on the unmodified
master
branch. I'm not seeing tests running in CI, so a maintainer may need to verify that this fix actually works as intended.