[WIP] Next basset version #143
Conversation
v2 - named assets, no more symlink, caching features
|
OMFG I absolutely LOVE the description here @pxpm . I LOVE IT! Brief but comprehensive, and crystal-clear. Let's use this as the changelog for this version - it's EXCELLENT. |
There was a problem hiding this comment.
Hey @pxpm - as much as I loved your description for this PR... the diff is difficult to read. man.
I've spent 2h reading this PR and still don't feel like I understand it at all.
Seems like EVERYTHING changed, everything became more complicated, and I'm struggling to understand the reasons for the changes, to be able to judge the decisions you've made. I'm sure it works, but I can't really accept this PR until I understand it. Until I know we get it to be cleaner than what we had before. And that will take us few back-and-forths, from what it looks.
-
First up, please take some time to reply to my answers here... everything is surface-level because (again) I don't understand it.
-
Second, please take some time to describe the changes to me, from a technical perspective. Broad picture. Eg.
- an asset used to be a string, but now it's an object and I called that object cacheEntry;
- our cachemap used to X, now it Y;
- this is new to me too... but there are just too many changes here, in every file... for me to understand WHY they have all changed; maybe a summary is possible, and that would help me... I don't know;
- If you have time, please also do a self-review. Erase everything from your memory and review this DIFF as if it were your first time trying to understand what happened here. See if you'd change anything for readability, clarity, brevity, etc.
We can do this 💪
There was a problem hiding this comment.
This is a very nice class from a programming perspective. The names seem good as well. Please add comments and explanations, so that it's easy for a human to read it as well. It's got 18 methods so feel free to split them up as well, to make it clear what the affordances are for this class.
| if (! file_exists(base_path('.gitignore'))) { | ||
| $this->components->task($message, function () { | ||
| file_put_contents(base_path('.gitignore'), 'public/'.config('backpack.basset.path')); | ||
| }); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
Wait. Why do we add the basset path to .gitignore on installation? Wasn't the whole point of publishing to the public directory... to have the assets in source control?
There was a problem hiding this comment.
Hey Cristian, my understanding from last iteration we had is that we would keep the "current behavior" as the default, but removing from .gitignore and pushing the public assets to the repo would work as if they were regular files, namely only "changed files" will appear in the diff.
We can change the default, but for that I would argue that we need a way to "don't load" some assets (things you are not using). I thought about Basset::map('asset-name', null) to don't load that specific asset, but it seems cumbersome, so maybe Basset::dontLoad(['asset-1', 'asset-2']) would be better (defined in the same OverrideAssets class).
But there is also another issue, if the basset isn't "named", so a @bassetBlock or something similar we would always load it, and I think that's not the desired behavior too. eg:
4 wysiwyg editors. they all load the "editor script" and the "editor initialization code". You only use 1 of them, so you could easily do Basset::dontLoad(['editor-1', 'editor-2', 'editor-3']). That wouldn't load those editors from cdns, but the "initialization scripts" for those editors would still be loaded. Similar issue with the CSS.
I think one reasonable solution would be to have "grouped assets", so that the editor.js, editor.css and the @bassetBlock('editor-initialization') could be disable in one go by disabling the whole group. This will work if we have "related code blocks/urls" in different files.
If we wanted to simplify things as everything is usually on the same file, we could add excluded_views that would not cache anything inside those files.
I think this is kind of a must when you are publishing assets to your repo, you wouldn't want files that you are not using, either for consistency or for saving space/computation time.
I didn't implemented nothing like that yet, maybe we can talk a bit about it and decide if it's worth to do right now and how to do it. 👍
There was a problem hiding this comment.
After further review, I agree with your decision here - .gitignore will work best in 99% of the cases, so we keep it as the default, yes.
For the assets-committed-to-git use case... I understand why a new function would be needed, yes. I don't think it's urgent to create one, but here are my thoughts:
Basset::map('asset-name', null);
// I like it, it simple and clean;
Basset::dontLoad(['editor-1', 'editor-2', 'editor-3']);
// I don't like the "dont", and don't like the "load" - does that mean it doesn't cache it or what? not explicit enough;
// maybe we can do something like
Basset::ignore('asset-name');
// but this still doesn't answer the whole question... what will happen...
// - will it not get cached on disk?
// - will it not get loaded onpage at all? or will it be loaded from the CDN?
Regarding grouping, I would NOT add grouping. That will add a lot of complexity to our code, for a 1% scenario. It would probably be easier to just add something like this, to allow vendors to create namespaces for names:
Basset::ignore('ckeditor.*');
But even that... to be honest, would add complexity, and I don't know if it's worth it at this point.
--
If you think we should do this... please add an issue for it with our best thoughts... but personally I don't think this is URGENT nor IMPORTANT so I wouldn't spend time on this right now.
src/BassetManager.php
Outdated
| // when in dev mode, cdn should be rendered | ||
| if ($this->dev && ! $this->forceUrlCache) { | ||
| $output && $this->output->write($asset, $this->dev); | ||
|
|
||
| return $this->loader->finish(StatusEnum::DISABLED); | ||
| } |
There was a problem hiding this comment.
I see too many places in the code where dev_mode is mentioned... and I don't understand how it works any more. Previously, dev_mode meant assets weren't actually internalized. Now... what does it mean?
Please explain to me:
- what
dev_modedoes in this current version; - why you should use it;
- why you would need to separately set
dev_modeandforce_url_cache;
There was a problem hiding this comment.
Dev mode has 3 tasks:
1 - do not cache external asset urls
2 - do not cache files located in accessible folders (eg: /public/my-assets/asset.js)
2 - refresh the "forced" local cached files. (files that are located in the vendor/ folder that need to be moved to accessible folders before used).
Using the force_url_cache will just change the behavior of the 1 task.
In addition to this change, dev mode will now also compute and compare the basset file hashes of the vendor files and only perform the internalization of the ones that changed.
There was a problem hiding this comment.
Sorry but just tried it and... I don't see that behaviour. And I still don't understand what dev_mode does now. This is too convoluted. Here's what I've done:
--
I've run php artisan basset:clear, have no basset entries in the .ENV file and here's my config file:
// development mode, assets will not be internalized
'dev_mode' => env('BASSET_DEV_MODE', env('APP_ENV') === 'local'),
// all external urls (usually cdns) will be cached on first request or
// when running `basset:cache` even if dev_mode is enabled
'always_cache_external_urls' => false,Due to the comments in this config file (and your previous comment in this thread), I expect that no assets will get internalized. That it will load from the CDNs directly. Because my APP_ENV=local and that's what the comment above dev_mode says "assets will not be internalized".
I open the page and... after I hard-refresh... I get a broken page:
and some assets DID get internalized (the vendor ones from what I can tell):
If I click "view page source" I see that the URLs the page tries to load are the "internalized URLs", but those files aren't there.
This was NOT my expectation. The change in behaviour of dev_mode doesn't make sense to me - I literally don't know how to use it. This new always_cache_external_urls config also doesn't make sense to me, especially in combination with dev_mode. It's an always... so if I turn it "false" is becomes... not-always... what does "not always" mean... in what cases is it enabled?! 😡
To me, the previous behaviour of dev_mode was easy to understand - you flip that switch, and the Basset will do nothing except the absolute mandatory. Now... I don't know what dev_mode does. And there's a new config that says it will basically ignore the previous config?! I can't wrap my head around it. And if I can't, nobody will.
Since your explanation still didn't clarify to me how I can use dev_mode... or why I should... it might be smart for me to wait for you to write the README. Maybe after you do that... I will understand what dev_mode does and when I should use it.
But if it's not a me-being-stupid problem, and more of a this-being-way-to-convoluted problem.... I might have a way out, a way to clarify what dev_mode does AND to customize it. Here goes. What if we:
- keep the BASSET_DEV_MODE entry in the .ENV file as the toggle;
- remove the
dev_modeentry from the config file - add config entries for every little thing DEV_MODE disables, and use the BASSET_DEV_MODE env as the default for them;
I think pseudocode will explain it better. Something like this:
// this entry goes away entirely
// 'dev_mode' => env('BASSET_DEV_MODE', env('APP_ENV') === 'local'),
// do not cache assets from external urls
'do_not_cache_external_urls' => env('BASSET_DEV_MODE') ?? false,
// do not cache files located in accessible folders (eg: /public/my-assets/asset.js)
'do_not_cache_files_in_accessible_folders' => env('BASSET_DEV_MODE') ?? false,
// do not trust cached code blocks (eg. loaded with @bassetBlock)
// instead of loading the cached code block, should check if it's changed and re-cache if needed
'do_not_trust_cached_code_blocks' => env('BASSET_DEV_MODE') ?? false,
// do not trust cached local files, eg. files that are located in the vendor/ folder and were moved
// to accessible folders before used; should check if changed and re-cache if needed
'do_not_trust_cached_local_files' => env('BASSET_DEV_MODE') ?? false,Wouldn't this clarify EXACTLY what dev_mode does? And give you granular control over it?
If we are to do this, please do it as a separate PR, I'm not 100% sure we should do it either.
…o next # Conflicts: # src/Helpers/FileOutput.php
There was a problem hiding this comment.
Hey Pedro! I love the PR in the end 😀 Took me a while to wrestle with it... In the future let's not leave so much work in one PR before we review... we're just making it too difficult to ourselves.
After using it and going through all the code, I think I understand it now - thank you for your patience. I noted here quite a few suggestions (most minor / DX).
In addition, please:
- add a new section to the README file, an UPGRADE GUIDE;
- go through the rest of the README file, to make sure it details the new behaviour (not the old one) and all of the things people can do with it;
| @@ -183,11 +189,10 @@ public function loadDisk(): void | |||
| } | |||
|
|
|||
| // add the basset disk to filesystem configuration | |||
| // should be kept up to date with https://github.com/laravel/laravel/blob/10.x/config/filesystems.php#L39-L45 | |||
| app()->config['filesystems.disks.basset'] = [ | |||
There was a problem hiding this comment.
Upgrading to this new behaviour isn't easy-peasy. A person would need to also delete the storage/app/public/basset directory... from all their environments. That's why I assume most people who already have it running... will just want to keep the old behaviour. It's working for them, after all.
Through this lens, this looks like a change in behaviour that:
- will confuse people;
- can't easily out-out of;
- can't easily upgrade to;
Don't get me wrong! I do agree with doing this, and with making it the default! But how about this:
- we leave the previous disk as it was, just rename it to
storage_basset; we keep it for backwards compatibility; - instead of changing that disk, we make create a new disk, that we call
public_basset(the default);
That way, we know we have the two options available for people, in v2:
- to can keep the old behaviour, make sure you have
BASSET_DISK=storage_bassetin your .ENV file or config; - to adopt the new behaviour, use
BASSET_DISK=public_bassetand delete thestorage/public/app/bassetdirectory from all your environments (it's that's no longer used);
| 'root' => public_path(), | ||
| 'url' => url(''), |
There was a problem hiding this comment.
Any reason not to make this disk use the basset directory by default? Seems like this is a disk towards /public not /public/basset. That means we're hardcoding the basset path elsewhere in our code, right?
There was a problem hiding this comment.
Later edit: I understood why, because of BASSET_PATH 🤦🏻♂️
| * @return void | ||
| */ | ||
| private function createSymLink(): void | ||
| private function addGitIgnore() |
There was a problem hiding this comment.
What do you think about making this optional? To ASK the user whether they want to do this, upon installation? That's a great moment to give them the choice, right?
Eg.:
Would you like to add the 'public/basset' directory to .gitignore? This means cached assets will not show up in your git history, if you use the
public_bassetdisk. (Yes/No; default: Yes) More info: https://bla-bla
There was a problem hiding this comment.
Then that could link to the section in the docs where we explain the benefits of each option:
- if you add this to .gitignore, you should have
php artisan basset:cacheto your deploy script; - if you do NOT add it to .gitignore, you are 100% sure you have the same assets in local and staging, but your git history will also have the changes to your CSS and JS dependencies;
| @@ -30,7 +30,7 @@ class BassetCache extends Command | |||
| * | |||
| * @var string | |||
| */ | |||
| protected $description = 'Cache all the assets under the basset blade directive'; | |||
| protected $description = 'Cache all the assets under the basset blade directive and update package manifesto'; | |||
There was a problem hiding this comment.
By package manifesto you mean the cachemap, right? Then let's call it that, everywhere.
| protected $description = 'Cache all the assets under the basset blade directive and update package manifesto'; | |
| protected $description = 'Cache all the assets using the basset blade directive and update the cachemap'; |
| @@ -20,8 +20,8 @@ class BassetServiceProvider extends ServiceProvider | |||
| Console\Commands\BassetClear::class, | |||
| Console\Commands\BassetCheck::class, | |||
| Console\Commands\BassetInstall::class, | |||
| Console\Commands\BassetInternalize::class, | |||
| Console\Commands\BassetFresh::class, | |||
There was a problem hiding this comment.
@pxpm can you please check that this command does what we expect it to do? Last I remember, it didn't.
I really want us to be able to tell people they can safely run php artisan basset:fresh - it's especially useful in the scenario where you do NOT have the files in gitignore. You don't only want to add new files, you also want to delete the old ones, right?
There was a problem hiding this comment.
Not directly to this PR, but here's something I discovered:
- the new ckeditor addon does NOT add its path to Bassets; so its asset aren't cached when you run
php artisan basset:cache; I assume it's the same for the new tinymce addon; - the CSS and JS we use in our Chart Widget (in PRO) is not internalized upon
php artisan basset:cache - FileManager does NOT add its path to Bassets, or at least when you load it, it caches more files;
These are the few cases I found we can probably fix. But my point here is this: I don't think it's SAFE for anybody to assume that php artisan basset:cache will actually cache everything you are using. I think we should heavily recommend that people use the .gitignore, unless they know what they're doing and want full control.
Why? Because if you are not using .gitignore for that directory... and you missed just ONE file on localhost that you didn't cache... the downside is big. Your deployment script stops working. So I am 90% sure people who go down this route... will end up with conflicts in production.
Let's fix the problems above in their respective directories (or leave this open until we do). But I wanted to take the opportunity to detail why I believe .gitignore should be our default and recommended option.
There was a problem hiding this comment.
That should be fixed in Laravel-Backpack/CRUD#5720 , https://github.com/Laravel-Backpack/PRO/pull/292 , Laravel-Backpack/FileManager#64 , Laravel-Backpack/theme-tabler#194 & other themes.
I may have missed some of them in other packages so we should carefully check them.
Theoretically we must ensure that a developer can do a basset:cache and all "Backpack" assets get cached, but for the reason you mentioned, I opted to have the .gitignore by default until some people conscientiously change it to have the assets in the repository and may find anything we've missed.
The aim, like I said, should be that EVERYTHING Backpack related can be cached manually.
| use Illuminate\Support\Facades\File; | ||
| use JsonSerializable; | ||
|
|
||
| final class CacheEntry implements Arrayable, JsonSerializable |
There was a problem hiding this comment.
Excellent work in this class! Love the name, love the code, nice and clean. The only inconsistency I've found is with the naming inside an entry:
Inside a CacheEntry, we have:
- asset_name
- asset_path
- asset_disk_path
- attributes
- content_hash
Aren't the last 2 also in relation to the asset? Why don't they have the asset_ prefix too?
There was a problem hiding this comment.
Alternatively... we could drop the asset_ prefix from ALL of them. In the end, that's what a CacheEntry represents... and asset, right? But I feel like that would be a bigger change 😅
Whatever you think is best here @pxpm , just highlighting the inconsistency
| private array $attributes = []; | ||
|
|
||
| private string $content_hash = ''; |
There was a problem hiding this comment.
Same here, why do the previous ones have asset prefix and these don't?
| return $this; | ||
| } | ||
|
|
||
| public function attributes(array $attributes): self |
src/Console/Commands/BassetList.php
Outdated
There was a problem hiding this comment.
This command... didn't work for me, it didn't output anything:
There was a problem hiding this comment.
The command totally works, but that list only the named assets, things that you can directly copy to your OverrideAssets class and change them. The other can't be changed without publishing some view and changing it.
For that to work you must be on the branches I mentioned in other comment: #143 (comment)
There was a problem hiding this comment.
Oohhh ok. Then let's have it output something (anything) in this use case. Maybe smth like:
- "No named assets have been found in the cachemap"
- "End of named assets list"
- "0/300 assets in the cachemap have names"
Whatever is easiest, but an output nonetheless.
What's new? (Summary)
bassetdisk, it points to the/publicfolder instead of storage, removing the need for symlink.public/{basset_path}to .gitignoreRemoving the symlink and adding .gitignore
This had been an issue for some of the users. The mangle between filesystem, permissions and symlinks had been a source of problems.
This PR removes the need for symlinking because the assets will be cached in the
/publicfolder. Since the public folder is not.gitignoredby default, the install command now adds the basset folder to the gitignore.Cache URL's in dev mode
One painpoint of using basset in development was that you couldn't cache the http dependencies (usually cdns) and keep your "local" files "uncached", so that you could work developing your admin panel without internet access.
In this PR we introduce the
force_url_cache, that will cache the urls even when you are in dev mode. That way it's possible to cache everything in advance withphp artisan basset:cache, or at the required time when the asset is saw on the page for the first time.Local files (recaching issues)
While working locally, all local assets (eg,
vendor/backpack/crud/resources/assets/css/common.css) were "re-cached" on every request. That was required to make the assets available "for public" usage (the vendor folder is not accessible).In this PR we introduced an "hash" that we store alongside the local files in the file map. That way, we only need to "re-cache", the ones were the content changed.
Named Assets
This is the block muscle of this PR. By using named assets we unlocked a lot of other possibilities.
The concept is simple, packages (like backpack/crud), register their named assets with
Basset::map(), when refering to those packages, developers can use only@asset('asset-package-name').The advantages of this approach are:
Basset::map()definition in 1 place, ensuring that no package conflicts occur by using multiple versions of the same package.All this now enables the workflow of publishing your assets to your repo, removing the need for calling
php artisan basset:cachein production. Or by using a combination of both, and just calling:cachewithout:clear, so only the "changed" assets will be cached. If nothing changed, no assets are "re-cached".