Laravel-Backpack / CRUD

Build custom admin panels. Fast!
https://backpackforlaravel.com
MIT License
3.04k stars 884 forks source link

[Feature Request] Bundle public vendor packages #3271

Closed danilopolani closed 3 years ago

danilopolani commented 3 years ago

Feature Request

What's the feature you think Backpack should have?

Bundle (out of the box) of public/packages in a single (minified) file. Currently the packages folder is ~3.7k files and this slows down every action where git is involved (e.g. my Github action to deploy the project takes ~2m only to copy the public files)

Have you already implemented a prototype solution, for your own project?

No

Do you see this as a core feature or an add-on?

Core

pxpm commented 3 years ago

Hello @danilopolani

Thanks for raising this issue, we are aware of some kind of problems with our bundle size.

We are trying to figure out the best solution for it, but ATM we can't provide a fix.

I am going to postpone this to 4.2 launch when we will go tackle "non-bug"/features/improvements as of now we are under active bug cleaning in 4.1 version.

If you have any solution that might work just share with us, we are open to ideas how we can improve.

Wish you the best, Pedro

danilopolani commented 3 years ago

Hi Pedro, sure no problem :) I thought about a solution but it's a bit dirty / not the best to be maintained, as soon as I have a bit of spare time (and not tired because of work) I'm gonna give it a shot and submit to you guys.

Thanks again

danilopolani commented 3 years ago

Ok, I found that the easiest way is to manually define required files, see this branch. There two big problems:

  1. The iconpicker package is fucked up. The tag 1.8.x does not have anymore a lot of icon packages. I tried to require the package at the commit 58154dfc0e8bd223efd1a01a7c7a54e766d827c5 (the latest with Fontawesome 5.x) but I failed.
  2. Language files of packages (all the WYSIWYG editors, moment and more) are the reason the "bundle" is big.

Maybe in a future version we could think of another way to achieve i18n on fields? Something like:

By default you only have english, if you want another locale you can run php artisan backpack:package:locale moment it-IT and it will automatically download (?) the needed files on your public folder"

And:

If you want to use specific fields (like ckeditor, tinymce etc.) you have to manually require the needed dependencies (php artisan backpack:package:require tinymce) and it will be downloaded. (This requires the user to have npm installed)

If you combine both solutions we can achieve a smaller backpack/crud package size and the final public/packages of the user will contain only its desired files. They're also backward compatible since old users already have all the needed files.

tabacitu commented 3 years ago

Hi @danilopolani ,

Like Pedro said, we'll definitely try to do something about this in 4.2 (so soon). But personally I don't think bundling all the assets in one CSS file and one JS file is a good solution. Even if we resolve the two biggest problems (languages and fonts):

For those reasons, I'm skeptical that bundling all field types is a good long-term solution. Personally I think that a better solution will be to:

  1. Move big/problematic fields into their own packages, as 1st party add-ons, so that only the admin panels who need them install them (ex: icon_picker, ckeditor, tinymce).
  2. Keep the possibility for each field to loads its own assets, separately. With HTTP/2 (and especially with HTTP/3) asset calls are really cheap, and the page won't wait for one asset to be downloaded for the next one to load; which means it's faster to load 10 files than to wait for 1 bundle file of the same size to load).
  3. Find a good solution to default to a CDN asset first, like we did in Backpack v3. But also have a really easy option for people who want to keep the assets in-house, which will pull the assets from the CDN and store them locally.

But then again, I've tried to prototype a solution for that and I can't say I'm 100% happy with any of the results. Hopefully when we get to spend time on this alone, and make it a priority, we will. It's a really tricky thing to do. But that also makes it exciting 😄

Like Pedro said, we're currently focused on finishing up 4.1, fixing all the non-breaking bugs. Next up we'll take a look at this. I've just dumped my thoughts on you about bundling vs unbundling in order to be fully transparent and get early feedback.

Cheers!

danilopolani commented 3 years ago

Ehy @tabacitu , yeah bundling everything is not a good idea, in fact in my previous answer I went with another way (trying to remove useless files from being copied).

I agree with you about the solutions, if you read my comment I "drafted" an idea like your first 2 points, that is providing user ability to install only needed packages field-per-field reducing assets and installing only needed ones.

Anyway I don't want to press you guys for v4.1 haha, this was just a discussion for a future version, you're doing already a great job!

tabacitu commented 3 years ago

Update: still looks like bundling might not be the best option here, but loading files only if they're not already loaded.

Let's close this in favour of https://github.com/Laravel-Backpack/CRUD/pull/2652 since that looks like the most promising solution right now. And move the conversation there. I'm pretty sure there are other threads about this too... it's probably the top problem we need to solve in 4.2.

Thanks again for raising this @danilopolani . Cheers!