search

ElMassimo / vite_ruby

⚡️ Vite.js in Ruby, bringing joy to your JavaScript experience
https://vite-ruby.netlify.app/
MIT License
1.28k stars 117 forks source link

fix: Retain bundled current files (close #438) #477

Closed benlangfeld closed 3 weeks ago

benlangfeld commented 2 months ago

This was broken in https://github.com/ElMassimo/vite_ruby/commit/8a581c15ff480049bbb14dab1b5a3497308521b5 with no explanation of why it was being removed.

See #404 and #438 .

benlangfeld commented 2 months ago

@ElMassimo I'd very much appreciate your feedback on this and that it make it's way to the next vite_ruby release. This bug lead my company to 30 minutes of lost sales this week, and I'm trying to redeem myself quickly 😅

philippevezina commented 2 months ago

@benlangfeld, you should remove the "Closes" keyword as it does not actually fix the reported issue: https://github.com/ElMassimo/vite_ruby/issues/438#issuecomment-2210934756

benlangfeld commented 1 month ago

@ElMassimo Any chance this could be reviewed? It fixes a critical bug in this gem and deserves to be released urgently.

ElMassimo commented 1 month ago

I'd like to merge a fix that fully addresses #438.

The main problem is that—unlike Webpack—Rollup will create files with different mtime, so we should cluster them in order to have clean with the default parameters (used implicitly through assets:precompile) working as expected in most scenarios.

Leveraging the manifest to keep referenced files is nice (your patch fixes the current implementation), but it's not sufficient to address the problems in the current clean implementation.

I'm also considering removing clean altogether, as it has the potential of breaking existing deployments, and let each application handle non-image-based deployments as needed. A separate gem that is tailored for capistrano or similar deployment tools can easily extend assets:clean, so this doesn't necessarily need to live in vite_ruby.

benlangfeld commented 1 month ago

@ElMassimo Does a more complete fix exist? Assuming it does not, why not merge this in the meantime? Right now the implementation is fundamentally broken and dangerous. Indeed, there are fixes for bugs that are not described in #438 included here.

benlangfeld commented 3 weeks ago

Well, looks like this feature was removed in https://github.com/ElMassimo/vite_ruby/commit/824b4ef8397828423d2ddda117bf27e365954961.