Improve webpacker:clear task

[gauravtiwari]

Fixes: #2441

By default it will keep the latest version, 2 backups created within the past hour (same as sprockets). To force only 1 backup to be kept, set count=1 and age=0. To only keep files created within the last 10 minutes, set count=0 and age=600.

bundle exec rails "webpacker:clean[1, 0]"
bundle exec rails "webpacker:clean[0, 10]"
bundle exec rails webpacker:clean

[sbeam]

This still assumes that all the files for a given compilation run of webpack are going to be outputted within 1 second, and so can be grouped into distinct runs and counted that way. That's not a valid assumption.

The max_age code helps, but this still could lead to confusing and incorrect behavior. If files are older than the age param, they could still get grouped incorrectly and deleted prematurely.

Also, with default capistrano config or heroku buildpacks or docker deploys, users don't often have the option of (easily) changing the params given to rake assets:clean, or passing that param along to the webpack:clean task. So it's probably still important to make sure that the count method is reliable.

[justin808]

@sbeam see my comment above. How about cleaning before compilation, rather than after with all this logic?

[sbeam]

@justin808 that would work for some situations (including mine), but as I understand it, the point of keeping the last 2-3 versions of each compiled asses is in cases of rolling deploys or clients that have partially cached some of them, and there are still references to them out in the wild. So suddenly 404ing all old assets on each deploy isn't good. This is potentially an issue if you have e.g. capistrano deploys to multiple servers behind a naive load balancer that complete at different times.

That's why sprockets has similar code more or less for years.

[sbeam]

@justin808 if you do want to clean the dir entirely, I think you could use the clean webpack plugin

[justin808]

@sbeam I think for rails/webpacker, that can work. However, I generally prefer that the webpack config be solely focused on webpack builds and not things like deleting files from some directory. In other words, I find plain shell commands more descriptive. Another issue is that many React + React + SSR projects build the client and server bundles in the same directory. So some thought must be given the order of the webpack builds, if concurrency is used, and ensuring that the clean stuff is configured right.

[justin808]

@gauravtiwari how does one set the params that clean will use after precompilation is completed?

Also, would it make sense to allow whitelisting some files so that they are never cleaned?

In the case of server-side rendering, there is no need to put a hash on the bundle name. But I just ran into this exact issue a few times where the precompile calls the clean and that deletes the server bundle: shakacode/react_on_rails#1245

I can advise users of React on Rails to always put a hash on the server bundle.

Alternately, would you be open to a PR that allows some files to be whitelisted from cleaning, maybe in the webpacker.yml file? or an ENV value? That way, a file by the name of server-bundle.js would never get deleted.

[justin808]

@gauravtiwari if you really want to clean the directory, saving what Webpack created, why not just skip the whole clean and instead clear the destination directory before the build starts?

[somebody32]

I was investigating this code today and I think those comments are misleading (or I'm mixing booleans in my head).

if we pass age as 0, then max_age will always be bigger, so the whole max_age < age && index < count will be false always and then everything will be removed independently of the value of index.
In this case, age should be something like Float::INFINITY

the same story for the second line, you can't pass count as 0, should be infinity too

[somebody32]

Maybe it is worth a separate issue but I also find relying on mtime to be brittle. Depends on how you structure your packs some of them could not be updated for a really long period of time and then the next deploy touching them could potentially have some period of time with 404s.

Sprockets + capistrano are backuping the manifest file, maybe webpacker could do the same? And then remove everything that is not a union of the current manifest and the backed-up one.

[viniciusgama]

I just run into issue @somebody32 pointed. The clean task did not retained the 2 latest versions of a file. It keep the one currently in use but not the previous one and that generated some 404s for us. That happened because like he said, that file was not modified in the last 2 deployments, but then when it was, it was not part of the recently modified files.

[benlangfeld]

In what way is the age of the retained files important? This just causes asset availability downtime and doesn't include any explanation of why such a thing is desirable.

[justin808]

@gauravtiwari @sbeam, maybe we can use the versions so long as they are OLDER than the age?

Right now, as soon as we find a few files with timestamps that differ by a second, we delete everything that's not in the current manifest.

[dkniffin]

Heads up for anyone else looking into this, there seems to be a fix here, but it's currently not available on 5.X.X, only 6.X.X, which hasn't been released (in non-beta) yet.

[Pezmc]

@dkniffin @viniciusgama #2912