Assets cache doesn't "notice" changes in imported scss files in development mode

[vovayartsev]

Hi,
I'm using sass to compile multiple css files into a single application.css, and in development mode the compiled file is cached, and subsequent changes to @import-ed have no effect.

// application.css.scss
@import './colors.css.scss';

// colors.css.scss
body { background: green }

Changing body color has no effect until I rm -rf public/assets or touch application.css.scss

Reproduced in v0.2.1.

[jodosha]

@vovayartsev Hi, I'm sorry about your problem. Are you using hanami-assets alone or with Hanami full stack project?

[jodosha]

Possible duplicate of hanami/hanami#513

[rafaels88]

@jodosha I am a little bit confused about how to create an integration test for this. So, before spend sometime doing the things wrongly, do you think it worth to create a predefined precompiled css file in public/assets in fixtures/static_assets_app/public/assets/ and check if it changed during the test?

[jodosha]

@rafaels88 I'd do this way:

1. Create a new fixture for application.css.scss
2. During the execution of the test, write on the fly a new file (colors.css.scss) in temp directory
3. Render application.css.scss and assert that it includes the contents from colors.css.scss
4. Modify on they fly colors.css.scss
5. Render application.css.scss again and assert that it includes the new contents from colors.css.scss

[rafaels88]

👍

[rafaels88]

So, @jodosha. I have spent sometime trying to understand somethings here but there is one thing which I could not understand. About Hanami::Assets::Compiler#fresh?, what is the real purpose of this method? I mean, what it is testing? I got confused about it because it seems a little different than Hanami::Static#_fresh?, or I am wrong? I mean, about the intention.

Well.. let me say what I am trying so far...
I tried to test if the problem was only in Hanami::Static#_fresh?. So I tested (a bad implementation, I know, but I was just playing with all the things):

def _fresh?(original, destination)
      return true if original.nil? || !::File.exist?(original.to_s)

      destination_content = _render(destination)
      original_content = _render(original)

      Digest::MD5.hexdigest(destination_content) ==
        Digest::MD5.hexdigest(original_content)
    end

    def _render(pathname)
      if pathname.to_s.end_with? '.sass'
        require 'tilt/sass'
        Tilt.new(pathname).render
      else
        File.open(pathname).read
      end
    end

After this, #_fresh? method responded with the correct answer, but I got stuck in Hanami::Assets::Compiler#fresh?, which respond the opposite and the compiler never run.

Could you help me to understand this?

Also, I was thinking about the best way to check for @import inside .sass files. I can simply check for @import string inside the file in a new class in Hanami::Assets. Is that the way you think is the best? I think it could get us to have a complex structure to verify "imports" and "requires" inside of different types of assets. But since this is my first time trying to resolve a problem in Hanami, I do not know the best approach to take here. Are you thinking about a specific implementation or do you want me to think in something and I ask you again what do you think?

Thank you very much for your patience and I am really sorry for all of this!

[jodosha]

@rafaels88 Thanks for your investigation.

The purpose of Compiler#fresh? is to avoid to compile again assets that didn't changed from the last time. Let's say you have 10 CSS, you are editing 1 and reloading the page to see the changes. You just want to compile again that 1 that changed, and not the other 9 that didn't changed.

By looking at its implementation we have two cases that makes #fresh? to return false:

1. The destination fine doesn't exist. This is the first attempt to compile that asset, and we want it to be compiled
2. The source file (eg. application.js.es6) changed, so we want to recompile the asset again.

---

Your implementation of freshness is not efficient because you always compile the source files. But the purpose of that algorithm is to avoid to compile when not necessary.

---

The real problem is that compile process is tight to HTTP requests. We always ask for /assets/application.css and if the corresponding source file (application.css.scss) didn't changed, #fresh? fails in its job.

[jodosha]

@rafaels88 I have a solution that is worth to try.

This is the evidence that we need to threat Sass files with special attention. In Compiler we already have special code for Sass, but at this point it we need to extract a SassCompiler before to proceed.

That means Compiler.compile should take the decision to choose itself or SassCompiler to do the job.

---

When we have that code in place, we can specialize SassCompiler behaviors.

First of all, when we compile a Sass file, we should specialize #cache!:

class SassCompiler < Compiler

  private

  def cache!
    super

    sass_load_paths.each do |path|
      cache.store(path)
    end
  end
end

In this way we're keeping track of the last modified time of all the sources where @import files are stored.

---

At this point, when we've been asked for application.css.css, we can use a specialized version of #fresh?.

def fresh?
  super || load_paths_fresh?
end

def load_paths_fresh?
  sass_load_paths.all? do |path|
    cache.fresh?(path)
  end
end

---

Please verify if this is a viable solution. Thanks! 👍

EDIT: There is one case that would make this inefficient. If apps/web/assets is a Sass source, then often #fresh? will return false, by causing all the Sass files to be compiled even when not necessary.

[rafaels88]

Hey @jodosha, thank you very much for the gentil and quick answer! I am feeling like making the things slower than it could be with my non-understanding, so, I am sorry for that! But I swear that after that, the next issue will be more quick! =)

So... here is my problem now:

Following the situation where I do request /assets/application.css twice, while modifying imported sass on the fly, we have:

/* application.css.sass */
@import './colors.css.sass'

/* colors.css.sass */
.white
  color: white

1. get /assets/application.css;
2. Static will check for freshness. Assets::Compiler have not been called yet. It will decide to compile after figuring that its precompiled file does not exists;
3. Static calls Assets::Compiler to compile application.css.sass with colors.css content inside its content. Great;
4. another get /assets/application.css;
5. Static will check for freshness. Assets::Compiler have not been called yet. It will check its modification time, Assets::Compiler will not be called.
6. Old version (item 3) will be served. Here is the issue.

So, my thinking is:

1. All logic that you wrote above will be called only until item 3. After that it will not;
2. Don't you think that Static should call Assets::Compiler to check freshness, instead having its own logic? I mean, is not this responsability from Assets?

I am pretty sure that I am missing something pretty obvious. And, I am sorry again for making all this fix so slow =/

[jodosha]

@rafaels88 Thanks for your detailed explanation. This reply comes later than usual because I was sick.

You're absolutely right: the check on Static will always return true and Assets::Compiler won't be called. I think that we should make this to work here first (maybe trying with my suggestion above) and then to figure out how to make it working with full stack apps in hanami.

[rafaels88]

👍 No problem, and thank you very much for the reply!

I will come with more news soon.

[jodosha]

@rafaels88 This is a gentle reminder about this issue. Thanks.

[rafaels88]

I am sorry, @jodosha. Getting back to this! I was terribly busy in the last days.

[jodosha]

@rafaels88 FYI, I'm working on a fix.

[jodosha]

Fixed by the following commits:

• 008446d
• 5de089f
• 91dc840
• 71fe129
• ee413cc
• 6629382
• e42b140
• d0cc84f
• c433b4b

Please note that I haven't created a PR because i committed+pushed the first three commits directly on master by mistake. Then I kept fixing it on master. Sorry for this error.