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

Wrap SassC/Sass gems #108

Merged
merged 6 commits into from Sep 13, 2019

Conversation

@landongrindheim
Copy link
Contributor

commented Sep 4, 2019

This is in response to a bug which was pointed out in
#106. We added require 'sassc',
which turned out to be a breaking change. If a user doesn't have sassc
listed in their Gemfile, they'll get load errors.

In order to resolve this, I'm opting to wrap both SassC and Sass,
with the idea that we can remove the reliance on Sass soon (the
sass-ruby gem is past end-of-life).

Wrap SassC/Sass gems
This is in response to a bug which was pointed out in
#106. We added `require 'sassc'`,
which turned out to be a breaking change. If a user doesn't have `sassc`
listed in their Gemfile, they'll get load errors.

In order to resolve this, I'm opting to wrap both `SassC` and `Sass`,
with the idea that we can remove the reliance on `Sass` soon (the
sass-ruby gem is past end-of-life).
@landongrindheim

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

Code coverage goes down due to

          require 'sassc'
          ::SassC
        rescue LoadError => exception
          begin
            require 'sass'
            ::Sass
          rescue LoadError
            raise exception
          end

Not sure how to account for the case where sassc is not available (tried stubbing the require call, but it was unsuccessful and felt wrong). Open to any tips on that 🙂

@jodosha jodosha self-requested a review Sep 4, 2019

@jodosha
Copy link
Member

left a comment

@landongrindheim Thanks for this PR. 💚

Did you also try to manage the loading of SASS via Tilt? If so, why did you pick this way and not the other?

NOTE: I'm not suggesting you to do so, just suggesting to explore all the options on the table.


If we decide to adopt this approach, we should port the other compilers with the same approach: wrap into an engine.

@landongrindheim

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

@jodosha I chose to go this route because Tilt doesn't implement #dependencies (or expose a method which would do that AFAICT).

I suppose I chose this method in order to preserve the dependency caching done when the compiler uses Sass. If we used Tilt, we'd have one fewer direct (gem) dependency, but compilation would take longer when SassC was not available.

Will Tilt rely on one's Gemfile for the presence of the Sass[C] source code? If so, that's a layer of another layer of indirection.

@jodosha
Copy link
Member

left a comment

@landongrindheim I left inline comments for issues to be fixed. Please remember to extract an engine for Less as well, even if it will use Tilt, but for consistency sake, it's better.

Would you please to take care of this following iteration? Thanks in advance.

lib/hanami/assets/sass/engine.rb Outdated Show resolved Hide resolved
spec/unit/hanami/assets/sass/engine_spec.rb Outdated Show resolved Hide resolved
lib/hanami/assets/sass/engine.rb Outdated Show resolved Hide resolved

@jodosha jodosha added the fix label Sep 11, 2019

@jodosha jodosha added this to the v1.3.3 milestone Sep 11, 2019

Remove support for Sass
Support for Sass was dropped in Assets v 1.3.2. No need to re-introduce.
Wrap Less engine (Tilt) for consistency
Since fd4bfcc, the interface for the Sass compiler has been wrapped
inside an object we own. In order to be consistent with our compilers,
the same approach is being taken here.
@jodosha
Copy link
Member

left a comment

@landongrindheim Sorry for the back and forth on this.

Given the engine idea was born to abstract sass and sassc, but now that we don't support sass anymore, we can simplify the code, by removing the engine at all.

Please have a look at this diff:

diff --git a/lib/hanami/assets/compilers/sass.rb b/lib/hanami/assets/compilers/sass.rb
index 28cc0ed..3e0f92e 100644
--- a/lib/hanami/assets/compilers/sass.rb
+++ b/lib/hanami/assets/compilers/sass.rb
@@ -1,5 +1,3 @@
-require 'hanami/assets/sass/engine'
-
 module Hanami
   module Assets
     module Compilers
@@ -18,13 +16,20 @@ module Hanami
           name.to_s =~ EXTENSIONS
         end
 
+        # @since 1.3.3
+        # @api private
+        def initialize(*args)
+          super
+          require "sassc"
+        end
+
         private
 
         # @since 0.3.0
         # @api private
         def renderer
           @renderer ||=
-            Hanami::Assets::Sass::Engine.new(
+            ::SassC::Engine.new(
               source.read,
               syntax: target_syntax,
               load_paths: load_paths
@@ -34,7 +39,9 @@ module Hanami
         # @since 0.3.0
         # @api private
         def dependencies
-          renderer.dependencies
+          renderer.dependencies.map(&:filename)
+        rescue ::SassC::NotRenderedError
+          []
         end
 
         # @since 1.3.2

The compiler dynamically loads sassc. It it isn't in the app Gemfile, it crashes and I believe it's self-explanatory: you had a .sass/.scss file, but you haven't bundled sassc gem.

If it doesn't blow up at the initialize time, it's safe to refer to ::SassC constant, the way we use to do right now in master.

That makes both Sass and Less engines obsolete. Sorry again.

Would you like to take care of this? What's the best way to get in touch with you? Thanks.

@landongrindheim

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

@jodosha I'm glad you were able to look at this from that perspective. Without supporting both SassC and Sass, the engine concept is just indirection.

I'm reachable by @ing me on GitHub, or by email at landon dot grindheim at gmail dot com. It's been fun contributing to Hanami's repos. I hope to do more in the future 😄

@landongrindheim landongrindheim force-pushed the landongrindheim:wrap-sass-gems branch from 4d681c0 to 81f598d Sep 12, 2019

@landongrindheim

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2019

@jodosha Looks like the build was cancelled. Are you able to retry it?

Edit: I'm going to amend the last commit and force push to trigger the build.

Back out Sass/Less engine changes
As pointed out by @jodosha, the wrapped engine concept was introduced as
a way to deal with Sass *and* SassC being dependencies. Since we've
dropped support for Sass, the engines are just a layer of indirection.

@landongrindheim landongrindheim force-pushed the landongrindheim:wrap-sass-gems branch from 81f598d to bfb025b Sep 12, 2019

@jodosha jodosha merged commit 1754cb0 into hanami:master Sep 13, 2019

2 of 4 checks passed

codecov/patch 83.33% of diff hit (target 97.83%)
Details
codecov/project 97.74% (-0.09%) compared to 31207b3
Details
continuous-integration/drone/pr Build is passing
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jodosha

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

@landongrindheim Thanks for your help! I appreciated that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.