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

sassc should be a runtime dependency #106

Closed
wants to merge 3 commits into from

Conversation

@glaszig
Copy link

commented Aug 20, 2019

since #102, there's require "sassc" in the code now which makes the gem a runtime dependency.

see #102 (comment).

sassc is being depended on during runtime
make it a runtime dependency in the gemspec
@jodosha

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

@landongrindheim

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2019

I'm disappointed that the implementation #102 is thought of as a mistake. I want to make this right 🙂

As @glaszig noted in his #102 comment, Tilt does attempt to load SassC, but falls back to Sass if that library is not available. With SassC listed only as a development dependency of this library, it seems likely that it will often not be available. With Sass being beyond end-of-life, what's the reason for not wanting SassC as a runtime dependency?

Before using SassC directly, the Sass compiler was referencing both Tilt (for compilation) and Sass (for caching dependencies).

@jodosha Would you like to see a move back toward Tilt? What can I do to make sure these changes are aligned with your vision for Hanami?

@cllns

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

@landongrindheim The rationale behind not requiring sassc as a runtime dependency is that people can use hanami-assets without using Sass at all. It's not about Sass vs SassC, it's more like Sass vs Less or people who want to use hanami-assets for JS but not stylesheets.

I'm not sure this is a bug, I might be missing something though. Users will have to change gem 'sass' to gem 'sassc' in order for this to work (and run bundle install). Did you do that at @glaszig? I couldn't tell from your comment on #102 whether you made that change.

@glaszig

This comment has been minimized.

Copy link
Author

commented Aug 26, 2019

@cllns

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Sorry, I'm still not clear. Does it work after changing it to gem 'sassc' and running bundle install?

I understand it's unexpected for a patch level version bump (and technically breaaking), but it's a trivial enough change (IMO) if it works.

@glaszig

This comment has been minimized.

Copy link
Author

commented Aug 26, 2019

Does it work after changing it to gem 'sassc'

oh sorry. yes, it works.

@landongrindheim

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2019

@cllns Thanks for the explanation 😄 That makes sense!

Would it make sense to do something like what Tilt does and use require within the body of the compressor/compiler namespace?

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

    def prepare
      @engine = Sass::Engine.new(data, sass_options)
    end

I'm happy to do this if you're interested and not already doing it 🙂 It sounds like leaving it as is is also an option.

Sorry for the oversight on this.

@jodosha

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

@glaszig

i naivly upgradet to 1.3.2 because it was a patch-level version bump and nothing indicated (to me, fwiw), that something else needs to be changed.

Sorry for not meeting your expectations. I was torn about switching from sass to sassc suddenly, in a patch level release. For a second I thought to release that change as 2.0.0, not to 1.3.2, but then I realized that this isn't an API change. An external dependency ceased to exist (sass) and a new one is replacing it (sassc). If we need to bump major release anytime SASS or any other compiling lib changes that will get potentially out of control.

So I feel good that we picked 1.3.2, it should've been not requiring sassc regardless of people using it or not.

@jodosha

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

@landongrindheim

No need to be sorry, you and @cllns did a great job!
@cllns nailed it: it's not sass vs sassc, but using SASS at all. People may not need to use it, or prefer LESS.


The solution that you proposed can work: we may want to dynamically do require "sassc" once our Hanami::Assets::Compilers::SassCompiler is asked to compile a .sass file.

We may also want to consider to use Tilt for that. Please investigate both the solutions and let us to know. Thank you in advance.

@jodosha jodosha self-assigned this Sep 1, 2019

@jodosha jodosha added the bug label Sep 1, 2019

@jodosha

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

@glaszig Another option is that you change this PR, in the light of the discussions that we had here. Please let us to know. /cc @landongrindheim

@jodosha

This comment has been minimized.

Copy link
Member

commented Sep 1, 2019

In either cases, I think this should be addressed sooner than later, because 1.3.2 is now broken for non SASS(C) users.

@jodosha jodosha added the fix label Sep 1, 2019

@jodosha
Copy link
Member

left a comment

We want to dynamically require sassc, if needed.

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

@jodosha

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

@glaszig Thanks for your changes. Did you try to delegate the require to tilt? See the LessCompiler as an example.

NOTE: I'm not suggesting to choose Tilt for this job, I'm asking if you tried and what's the outcome.

@glaszig

This comment has been minimized.

Copy link
Author

commented Sep 4, 2019

landongrindheim added a commit to landongrindheim/assets that referenced this pull request Sep 4, 2019
Wrap SassC/Sass gems
This is in response to a bug which was pointed out in
hanami#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

commented Sep 4, 2019

I opened #108 as an alternative approach. In that PR, I opted to wrap the gems, doing something along the lines of what Tilt is does. Totally fine with closing it since this PR was open first and may be the preferred approach 😄 Just let me know!

@jodosha

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

@glaszig Thanks again for this PR, I'm closing this in favor of #108 as it has a more complete approach to the problem. Your input and collaboration is very welcome on the other PR. Thanks.

@jodosha jodosha closed this Sep 11, 2019

@jodosha jodosha added wontfix and removed bug fix labels Sep 11, 2019

@jodosha jodosha removed this from the v1.3.3 milestone Sep 11, 2019

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