Webpack compilation digest is written even if compilation fails

[justin808]

In PR 2117, we changed to write the compilation digest always even if webpack totally fails.

See: #2117 (comment)

I wrote:

@gauravtiwari @jakeNiemiec Hi! I think this should be changed to check that at least the manifest.json was written.

If there's some error on the webpack build, the cache digest is still saved. Then you go to update your webpack config and it's confusing that there is no recompile.

If somebody wants TS suggestions, that can be done by editor configuration or other things separate from the webpack build process.

I can't think of a valid case where the digest should get written if the output of webpack is non-zero.

This test seems incorrect to record that the compilation is fresh regardless of status

https://github.com/rails/webpacker/blob/master/test/compiler_test.rb#L47

  def test_freshness_on_compile_success
    status = OpenStruct.new(success?: true)

    assert Webpacker.compiler.stale?
    Open3.stub :capture3, [:sterr, :stdout, status] do
      Webpacker.compiler.compile
      assert Webpacker.compiler.fresh?
    end
  end

  def test_freshness_on_compile_fail
    status = OpenStruct.new(success?: false)

    assert Webpacker.compiler.stale?
    Open3.stub :capture3, [:sterr, :stdout, status] do
      Webpacker.compiler.compile
      assert Webpacker.compiler.fresh?
    end
  end

[clemens]

I ran into the same issue recently. I can see how this makes sense when a compilation error is caused by the source files themselves (e.g. a syntax error) but there are other factors that contribute to the success of compilation. In our case, the sequence roughly went like this:

• A new package was added to package.json which needed Node >= 12
• Node 12 was available on developer machines but not on our servers
• Consequently, asset compilation failed
• Re-running the asset compilation command after installing a newer Node version told us that there's nothing to do because the digest hasn't changed

The issue becomes even more odd considering that the behavior of bin/webpack vs. rails webpacker:compile differs – the former always compiles the packs without a freshness check while the latter checks for freshness first and only if it determines the assets to be stale, it triggers compilation.

So in short:

• I think the current behavior is fine for compilation errors that are caused by source files (e.g. syntax errors) because fixing these compilation errors requires changes to the source files, which in turn would cause the digest to become stale.
• I think the current behavior is NOT CORRECT for other compilation errors (e.g. errors related to missing OS packages on the server), because there the fix doesn't involve changing the source files and hence the digest never becomes stale.

Is there any desire to change this behavior to something that is more robust towards compilation failures that are not related to the source files?

[justin808]

@clemens I agree. The digest needs to account for files like package.json, if it doesn't right. now.

However, maybe a better approach to use timestamps? React on Rails has used that for years. If any file, such as package.json, has a newer timestamp than the manifest.json timestamp, then a rebuild is necessary.

https://github.com/shakacode/react_on_rails/blob/master/lib/react_on_rails/test_helper/webpack_assets_status_checker.rb#L28-L42

@clemens, @guillaumebriday any reason why timestamps on files would not work better? I'm pretty sure they would be much faster.

[clemens]

@justin808 Sorry for the delayed response. Judging from the code you've linked, this would result in exactly the same issue: If compilation fails due to something that's related to the overall environment (not the content of files), then changing the environment to fix it without touching any files would not change the modification time (that's used by find_most_recent_mtime) and thus not result triggering compilation because it's not considered stale.

Happy to continue the discussion in a shakapacker ticket, if relevant.