framework icon indicating copy to clipboard operation
framework copied to clipboard

[10.x] Add `--clean` option to `vendor:publish` command

Open gdebrauwer opened this issue 2 years ago • 6 comments

Fixes https://github.com/laravel/framework/issues/48964.

I have a package that uses Vite to build assets. When the package is installed in a project, you run the vendor:publish command to publish those assets. You might end up with the following assets in your public folder: Screen Shot 2023-12-11 at 16 27 27

When the package's assets change, you have to run the vendor:publish command again to get the new assets in your project. When you do that, you get the following result: Screen Shot 2023-12-11 at 16 27 51 Because the asset filenames change, the existing old files are not deleted and the new files are just added to the existing directory. This is a problem, as the old files will start to accumulate in that folder.

This PR fixes that by introducing a --clean option to the vendor:publish command. When you use that command option, any existing files in a published directory will be deleted. That way, you end up with the correct and new files in your project: Screen Shot 2023-12-11 at 16 31 30

gdebrauwer avatar Dec 11 '23 15:12 gdebrauwer

I'm wondering if we need to consider how this ties in with https://github.com/laravel/vite-plugin/pull/251 to give a holistic framework approach - or at least a consistent naming convention.

timacdonald avatar Dec 12 '23 00:12 timacdonald

I love this idea.

My concern is that the implementation will lead to issues during deployments.

Imagine you have a package that has published public/vendor/acme/scripts.abc123.js.

You update the underlying package and it now has a new file it wants to publish, which will be named public/vendor/acme/scripts.xyz789.js.

The following happens...

  1. A client sends a request to your app.
  2. A HTML response containing a reference to scripts.abc123.js is sent back.
  3. You do a deployment which includes an rm -rf public/vendor/acme/ before publishing the new files.
  4. The client's browser finish parsing the response.
  5. The client's browser now tries to load scripts.abc123.js, which 404s.

I would love if we could solve this issue. Making the switch over more atomic is not the solution, as depending on network speeds, it could be a few seconds before the request for the old asset is made.

I could imagine a world where, when publishing assets, we create a laravel.manifest file. This file contains a reference to all the currently published assets.

Then we can offer a vendor:clean / vendor:prune command that has the sole responsibility of removing orphaned files. This command can be run at the end of the deployment process, possibly with a short sleep in your deploy script, to protect against the race condition outlined above.

This is something that https://github.com/laravel/vite-plugin/pull/251 has considered and is built for.

timacdonald avatar Dec 12 '23 22:12 timacdonald

Then we can offer a vendor:clean / vendor:prune command that has the sole responsibility of removing orphaned files. This command can be run at the end of the deployment process, possibly with a short sleep in your deploy script, to protect against the race condition outlined above.

But that means you still will have the old files in your git repository and you have to remember to run that command locally after the deployment process so you can remove the old files from the git repo?

(btw, it was awesome meeting you at LaraconAU 😄)

gdebrauwer avatar Dec 13 '23 07:12 gdebrauwer

@gdebrauwer it was awesome getting to hang with you!

I'm think more when you wouldn't have these committed to your git repository and instead publish them as part of your deployment process.

Will circle back to this shortly and see if I can come up with something.

timacdonald avatar Dec 17 '23 22:12 timacdonald

@timacdonald clean-orphaned-assets binary is great, but I think that there should be artisan vite:cleanup (example name) counterpart for deploying prebuilt assets into environment where Node isn't available.

Edit: I should have read the conversation above beforehand)

daniser avatar Dec 20 '23 07:12 daniser

I agree this is a good command! We could use it in Forge.

Just wanna make sure it supports my concerns outlined. I plan to come back and get this going (unless someone else addresses the issues first)

timacdonald avatar Dec 20 '23 08:12 timacdonald

Please send this to 11.x instead, thanks.

driesvints avatar Mar 12 '24 14:03 driesvints