Skip to content
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

Merged
merged 1 commit into from
Jun 14, 2023

Conversation

jackjennings
Copy link
Contributor

@jackjennings jackjennings commented May 11, 2023

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

@jackjennings
Copy link
Contributor Author

@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
Copy link

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.

Suggested change
require "sassc" unless defined? SassC
require "sassc"

Copy link
Contributor Author

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.

Copy link

@pvande pvande left a 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. 😄

@RobinDaugherty
Copy link
Member

RobinDaugherty commented Jun 14, 2023

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
Copy link

tagliala commented Jun 14, 2023

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
Copy link
Member

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

@RobinDaugherty RobinDaugherty merged commit 8b1e2c9 into BetterErrors:master Jun 14, 2023
@tagliala
Copy link

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

@jrmhaig
Copy link

jrmhaig commented Jun 15, 2023

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.

inulty-dfe added a commit to DFE-Digital/publish-teacher-training that referenced this pull request Jul 12, 2023
  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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants