search

BetterErrors / better_errors

Better error page for Rack apps
MIT License
6.88k stars 437 forks source link

Require sassc only in development #520

Closed jackjennings closed 1 year ago

jackjennings commented 1 year ago

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 loading better_errors without needing to install sassc (see https://github.com/BetterErrors/better_errors/issues/516), and is an alternative resolution to https://github.com/BetterErrors/better_errors/pull/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.

jackjennings commented 1 year ago

@RobinDaugherty is this something you can confirm is working?

RobinDaugherty commented 1 year ago

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 sassc.

tagliala commented 1 year ago

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

image

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 rake release script to generate and commit a distributable css in the repo. I usually use rollup for this use case

RobinDaugherty commented 1 year ago

I was able to verify that this works, thanks for the help on this!

tagliala commented 1 year ago

@RobinDaugherty apologies for my previous message, I confirm that this approach works, thanks!

jrmhaig commented 1 year ago

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 sassc.

I have just updated this gem in our application and can confirm I can see errors correctly. Thank you for your work.