laravel / nova-issues

554 stars 34 forks source link

Tools styles/scripts should be cacheable by browser #1146

Closed m-lotze closed 5 years ago

m-lotze commented 5 years ago

The routes /nova-api/styles/* and /nova-api/scripts/* should send HTTP headers like last-modified and etag, so these resources can be cached by a browser. right now they send cache-control: no-cache, private, which prevents caching.

Ideally they send just a 304 if the file has not changed.

Also it would be good, if these routes are registered without the whole middleware stack if possible. i dont think they need authorization and it would make the requests for these files a bit faster.

tufankilicaslan commented 5 years ago

I use google page speed module over apache ( https://developers.google.com/speed/pagespeed/module/) to combine and reduce page requests. But i did not manage to combine nova tool scripts and styles . It's combining other css and scripts, but not includes nova tools.

https://www.modpagespeed.com/doc/filter-js-combine https://www.modpagespeed.com/doc/filter-css-combine

These scripts makes the page load heavy if you have for example 20 and more components. That means 20 css files and 20 javascript files.

does anybody knows how i can include nova tools scripts/styles combining with page speed module? is it possible to do that with the current structure?

m-lotze commented 5 years ago

@webhealer: I am not sure, but this may be related to the header cache-control: no-cache, private. the pagespeed module honors cache headers and therefore might not combine files, that send a no-cache header.

Another problem is package developers leaving the line Nova::style('...', __DIR__.'/../dist/css/field.css'); in their FieldServiceProvider even if they do not provide any css.

This results in many requests to empty css files in my case.

I know that this is not really nova's responsibility, but if Nova::styles() could be changed to the following, it would reduce the number of requests. In my case it reduces /nova-api/style/... requests from 13 to 5.

/**
     * Register the given CSS file with Nova.
     *
     * @param  string  $name
     * @param  string  $path
     * @return static
     */
    public static function style($name, $path)
    {
        if (file_exists($path) && filesize($path)) {
            static::$styles[$name] = $path;
        }

        return new static;
    }
tufankilicaslan commented 5 years ago

+1 your suggestion about cache control and css filesize check,but this control should not run everytime on page load, its using server resources while reading filesize.

m-lotze commented 5 years ago

Another way might be to comment out Nova::style('{{ component }}', __DIR__.'/../dist/css/field.css'); in the FieldServiceProvider.stub.

davidhemphill commented 5 years ago

Hey there! In order to keep this repository focused on bug reports, we auto-close feature requests and requests for help. Feel free to post your feature requests so others can discuss and add reactions. We'll keep an eye on them for later planning.

m-lotze commented 5 years ago

I made this command to combine all nova 3rd party scripts and styles to one file:

class CombineNovaTools extends Command
{
    /**
     * The console command description.
     *
     * @var string
     */
    protected $description = 'Combines all nova tools scripts and styles in nova-tools.[js|css] in public/[js/css]';

    /**
     * The name and signature of the console command.
     *
     * @var string
     */
    protected $signature = 'combine:nova-tools';

    /**
     * Create a new command instance.
     */
    public function __construct()
    {
        parent::__construct();
    }

    /**
     * Execute the console command.
     *
     * @return mixed
     */
    public function handle()
    {
        Auth::login(User::role(['admin'])->first()); // change to a user with full access to nova

        app()->handle(Request::create('/')); // or wthaever your nova root route is

        $this->info('combining nova tools js and css');

        $this->combineTools();
    }

    private function combineTools(): void
    {
        foreach (['allScripts' => 'js', 'allStyles' => 'css'] as $method => $type) {
            $content = '';

            foreach (Nova::{$method}() as $file) {
                $this->info($file);
                $content .= \file_get_contents($file);
            }

            \file_put_contents(public_path($type . '/nova-tools.' . $type), $content);
        }
    }
}

Then i use laravel-mix to create versioned files and publish and change novas layout.blade.php like this:

...
<!-- Tool Styles -->
<link rel="stylesheet" href="{{ mix('css/nova-tools.css') }}">
...
<!-- Tool Scripts -->
<script src="{{ mix('js/nova-tools.js') }}"></script>
...

This way the tools scripts and styles are cacheable by the browser and the loading speed increases quite significantly. This also prevents requests for empty stylesheets and authenticating every tool asset.

thaifani commented 4 years ago

Unable to locate Mix file: /css/nova-tools.css

atmediauk commented 4 years ago

Unable to locate Mix file: /css/nova-tools.css

@thaifani I ended up adding asset() without mix 😬

It's surprising this has been overlooked by the Nova Team. If things are not rolled in to Nova core and we're encouraged to build out our own tools (which is cool) this subsequently leads to more custom tools within a single project. We're on 15 packages already and with that, easily hit 18 second load times locally. Production is worse. The command above (as hacky as it is) reduces that to 3 seconds!

A well executed/ reliable implementation of it should be provided in Nova core.

Pamposgsk commented 3 years ago

hi, could you please give more details on how to achieve the above. I believe the above problem is very important to be fixed not only regarding speed but due to the fact that in production enviroment sometimes while loading the page it automatically redirecting to nova-api/scripts . I tried the function abova but it creates empty js and css files thanks in advanced

thaifani commented 3 years ago

I think that you must locate the app_url to the public folder not the root folder and make it as virtual host this answer assumes I understand your question !!

On Mon, Dec 7, 2020 at 1:25 PM Pamposgsk notifications@github.com wrote:

hi, could you please give more details on how to achieve the above. I believe the above problem is very important to be fixed not only regarding speed but due to the fact that in production enviroment sometimes while loading the page it automatically redirecting to nova-api/scripts . I tried the function abova but it creates empty js and css files thanks in advanced

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/laravel/nova-issues/issues/1146#issuecomment-739825560, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE43YZJJABXTQHMLAXM36TDSTSUTFANCNFSM4GLCVHLQ .

Pamposgsk commented 3 years ago

I created a js folder and css folder inside public folder and then I run the function.Is that correct? What do you mean to locate the app_url to the public folder.Can you share an example please?

atmediauk commented 3 years ago

I made this command to combine all nova 3rd party scripts and styles to one file:

class CombineNovaTools extends Command
{
    /**
     * The console command description.
     *
     * @var string
     */
    protected $description = 'Combines all nova tools scripts and styles in nova-tools.[js|css] in public/[js/css]';

    /**
     * The name and signature of the console command.
     *
     * @var string
     */
    protected $signature = 'combine:nova-tools';

    /**
     * Create a new command instance.
     */
    public function __construct()
    {
        parent::__construct();
    }

    /**
     * Execute the console command.
     *
     * @return mixed
     */
    public function handle()
    {
        Auth::login(User::role(['admin'])->first()); // change to a user with full access to nova

        app()->handle(Request::create('/')); // or wthaever your nova root route is

        $this->info('combining nova tools js and css');

        $this->combineTools();
    }

    private function combineTools(): void
    {
        foreach (['allScripts' => 'js', 'allStyles' => 'css'] as $method => $type) {
            $content = '';

            foreach (Nova::{$method}() as $file) {
                $this->info($file);
                $content .= \file_get_contents($file);
            }

            \file_put_contents(public_path($type . '/nova-tools.' . $type), $content);
        }
    }
}

Then i use laravel-mix to create versioned files and publish and change novas layout.blade.php like this:

...
<!-- Tool Styles -->
<link rel="stylesheet" href="{{ mix('css/nova-tools.css') }}">
...
<!-- Tool Scripts -->
<script src="{{ mix('js/nova-tools.js') }}"></script>
...

This way the tools scripts and styles are cacheable by the browser and the loading speed increases quite significantly. This also prevents requests for empty stylesheets and authenticating every tool asset.

With the above has anyone else had issues with only some of the assets being loaded? When dumping Nova::allStyles() or ``Nova::allScripts()``` in an artisan command some assets are missing but when calling the same command in a http request e.g dumping in the boot method of the NovaServiceProvider results in all the assets being listed correctly.

Any ideas what could be causing this?

github-actions[bot] commented 3 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.