Revert "feat(paths): allow assets to be found using their non digested path (#100)"

[brenogazzola]

Closes #143
Closes #139
Might close #117
Replaces #141

While this was a nice idea, once again issues show that trying to extract digests generated by bundlers causes compatibility problems. Let's revert so esbuild chunking works again and think of another way to do this.

@joker-777 Your PR seems to revert most of the changes here, so I think it might be a better idea to revert the entire thing. I've added you as a co-author.

I'm keeping this PR open for a while before merging in case anyone involved in the listed issues/PRs wants to provide further input.

[joker-777]

@brenogazzola Thanks, then let's revert it.

[midnight-wonderer]

Wow, what a coincidence. I am debugging the same issue at the moment.
Thanks, @joker-777

[brenogazzola]

With no further input from anyone, and having run this in production without problems for a while, I'm merging it

[aaronjensen]

What is the migration path for people that relied on this behavior?

[brenogazzola]

Hey @aaronjensen can you explain how you were using it so I can see if I can think of anything? The original contributor was using it for preloads for example.

[aaronjensen]

We use Vite, without vite_rails. We ended up having vite generate a manifest and already-digested assets and we look up the digested path using the manifest. It's a pain, but that's what we get for using javascript tooling.

[brenogazzola]

I'm not sure I understand what you are doing. You are saying instead of using propshaft's precompile to generate the manifest file you are having Vite do it?

[aaronjensen]

Uhm, kind of. We have something that works. Propshaft can’t control the digest on vite/rollup assets because of how they are loaded lazily from JavaScript without a rather sophisticated propshaft compiler

[midnight-wonderer]

Not sure if I should chime in.
But I want to point out that everyone here use some kind of javascript tooling. That's why we choose propshaft.

I have zero issue in my use case, apart from the bugs solved by this PR.
I heard about Vite; I know its name and having rough idea what it is.
I couldn't quite follow the conversation (maybe it just me?).
Adding more context might help.

For me, propshaft can't control digest of assets generated by javascript build tools sound about right as it should be.

P.S. I use good old webpack btw

[aaronjensen]

But I want to point out that everyone here use some kind of javascript tooling. That's why we choose propshaft.

No, on the contrary. Propshaft and import maps allow you to eschew JavaScript tooling, which is an appropriate response given how complex it has become.

When we are forced to use JavaScript tooling, we are forced to deal with the things we have to deal with. My only point was "this pain is to be expected."

For me, propshaft can't control digest of assets generated by javascript build tools sound about right as it should be.

🤷 If you are using esbuild to generate a single file, propshaft can digest things just fine. It's only necessary to have vite/webpack/whatever emit digests when the emitted JavaScript code refers to the files by name. If they never do that, you can let Propshaft do the digesting.

I have zero issue in my use case, apart from the bugs solved by this PR.

Glad to hear it. This PR allowed us to get rid of the silly double digesting we needed to do for our async chunks that Vite generated (chunk-abcdef-abcdef.digested.js) but it made it so we could no longer rely on Propshaft to find the digested version of the thing we were trying to load, which was the thing the original PR that was reverted was attempting to do AFAICT. We relied on that behavior. It was removed. We had to do work to stop relying on it.

When I asked if there was a migration path, it was to see if any thought had been given to the people that were going to be impacted by rolling something that people may have relied on. It sounds like not, it was just done, so we figured it out.

[brenogazzola]

@aaronjensen I apologize for the rushed revert. I assumed the feature was a complete failure and the only use case I knew of (preloads) was a minor thing so removing the feature would not be a problem.

We are in a bit of a frustrating place because the idea was good, but esbuild name collision is making things complicated. Each bundler behaves a different way and coming up with a way that works for all of them AND all use cases is not obvious (see the assets:clean problem)

[aaronjensen]

Thanks. I don't know if I am familiar w/ the particular assets:clean problem you are referring to (though I am familiar w/ the general problem of knowing which ones to clean once they are digested... this goes way back).

One of the things that would have made our work easier today was if there was a sanctioned way to add to Propshaft's manifest. It would be relatively straightforward to translate Vite's manifest into something that Propshaft could use I'm guessing. I just wasn't sure where to start and I didn't feel like coupling to Propshaft internals (especially given this recent break).

In case it's useful to anyone, here is how we solved our problem today:

module JavascriptEntrypointHelper
  def javascript_entrypoint_tag(name)
    name = "src/#{name}.tsx"

    path = Manifest.file_path(name)

    src = path_to_javascript(path)

    tag.script(src:, crossorigin: "anonymous", type: "module", defer: true)
  end

  # This manifest is produced by Vite, which adds the digest to assets. This
  # is necessary because files import other files and we don't have a
  # Propshaft compiler that replaces those references with Propshaft digested
  # references. We attempted to have Vite emit entrypoints that did not have
  # digests, but some of the code-split chunks refer to entrypoints, so we end
  # up with the same problem. Instead, we let Vite digest all of its assets
  # and then use its manifest to produce an asset path that Rails can use.
  # - Aaron, Kelly, Tue Oct 31 2023
  module Manifest
    def self.get
      @manifest ||= load
    end

    def self.file_path(name)
      manifest = get
      entry = manifest.fetch(name)
      entry.fetch("file")
    end

    def self.load
      text = File.read(path)
      JSON.parse(text)
    end

    def self.path
      Engine.root.
        join("app/assets/builds/manifest.json")
    end
  end
end

[midnight-wonderer]

I see now that our context is wildly different.
Our setup heavily uses code-splitting and dynamic imports with bells and whistles, not just compiling to a single file.

I am not sure if it applies to Vite or your situation, but the way we did it is this:

• We don't emit *.digested.* on the entry files but everything else. Like this:

{
  output: {
    filename: '[name].js',
    chunkFilename: '[id]-[contenthash:20].digested.js',
    sourceMapFilename: '[name]-[contenthash:20].digested.map',
    path: outputPath,
  },
}

The same principle also applies to CSS/SCSS.

• We emit manifest from Webpack, only for the entrypoint that is

new AssetsPlugin({
  filename: 'index.json',
  removeFullPathAutoPrefix: true,
  path: outputPath,
  prettyPrint: true,
  entrypoints: true,
}),

• The manifest looks similar to this

{
  "entry1": {
    "css": "entry1.css",
    "js": "entry1.js"
  },
  "entry2": {
    "css": "entry2.css",
    "js": "entry2.js"
  },
  "entry3": {
    "css": "entry3.css",
    "js": "entry3.js"
  }
}

Note that there is no *.digest.* in their names.

• We do have entry-loading helpers implemented (similar to what you are doing just now) by reading the manifest file.

  def asset_pack_tags(entry_name, js_options: {}, css_options: {})
    asset_index = AssetIndexCache.instance.cached
    entry = asset_index.fetch(entry_name, {})
    [
      *entry.fetch(:css, []).then do |files|
        Array(files)
      end.map do |file|
        stylesheet_link_tag file.delete_suffix('.css'), **css_options
      end,
      *entry.fetch(:js, []).then do |files|
        Array(files)
      end.map do |file|
        javascript_include_tag file.delete_suffix('.js'), **js_options
      end,
    ].join("\n")
  end

We used it like this:

<%== asset_pack_tags(:main, js_options: { defer: true }) %>

• For cleaning up, we have this in our Makefile:

cleanup:
	rm -f ./app/assets/builds/*.digested.*

We never have these double hashing issues you mentioned.

This is probably not the migration path you are looking for. However, at least we know that every person has different contexts. I don't even know if any of these apply to Vite, but it works wonders in my projects.

[aaronjensen]

Our situations are more similar than you think. The main difference is that rollup (which vite uses) will sometimes import entrypoints from chunks, which means I can't use the technique of allowing propshaft to fingerprint the entrypoints, unfortunately. It's rather annoying, but now that I have the ability to interpret the Vite manifest, it works for now. This particular app is a legacy app anyway. All our new stuff is much simpler -- import maps and regular old JavaScript without a compiler. In some cases we bundle, but that's about it.