search

nystudio107 / craft-vite

Allows the use of the Vite.js next generation frontend tooling with Craft CMS
MIT License
52 stars 16 forks source link

Allow styles to be included separately from scripts #31

Closed cmalven closed 1 year ago

cmalven commented 2 years ago

Is your feature request related to a problem? Please describe.

Importing styles via Javascript causes a flash of unstyled content on every page load in development and creates difficult-to-reproduce differences in styling and animation behavior between development and production versions of the site.

Describe the solution you would like

The ability to reference styles in Twig separately from scripts, in the same way you can do in a standard, non-Craft Vite setup.

Additional context

I've been building projects with Vite outside of Craft since the very early days of Vite 2, and I almost always pull my styles into the index.html file directly vs. importing them into my Javascript. For instance…

<!-- in index.html -->
<link rel='stylesheet' href='/src/styles/index.scss'>

vs.

// in index.js
import '/src/styles/index.scss';

This works perfectly, is a fully-supported behavior in Vite, and has a significant advantage over importing them in your Javascript entry file in that the injection of these styles are not blocked by Javascript and so there is no FOUC on every single page load in development. Importing in this way also does not prevent you from preprocessing your styles (you can see that I'm importing a Sass file above) and I have yet to find a single significant downside to this approach unless you are using some sort of CSS-in-JS approach which requires your styles to be imported via JS.

The FOUC content that appears on every page refresh in development isn't just annoying, it actually breaks animations and transitions that are supposed to happen immediately on page load, because those elements already exist and are rendered by the time the styles load and apply any initial state to them.

Relying on Javascript to import your styles also creates a distinct difference in behavior in development vs production, which makes it harder to troubleshoot edge-case issues around styling and animation behavior.

It would be amazing if Craft Vite would allow me to insert my styles in Twig separately from my scripts in the same way I'm able to do in a standard Vite setup.

<head>
{{ craft.vite.link("src/styles/index.scss") }}
{{ craft.vite.script("src/scripts/index.ts") }}
</head>

It looks like similar requests have been made in other issues, but none of them for the exact same reasons as mine. Hopefully this rationale makes sense.

Thank you for your hard work on bringing Vite to Craft!

khalwat commented 2 years ago

This isn't how Vite works in terms of separating the CSS from the JS. It's not a build tool like Gulp or Grunt, it's a bundling tool for JavaScript.

Yes, if you're certain what the built name of the CSS will be, and you're not using any content hashes, you can hardcode it as you mention in the post, but if you start using a manifest with content hashes, it doesn't work quite so well.

While I could expose a function to walk the manifest looking for CSS files (the code is actually already there, it just isn't exposed), I'm not sure if it's the best way to do it.

In any event, the FOUC is happening because by default, CSS is loaded async, because we're assuming you are using Critical CSS as a best practice.

However, if you don't plan to use Critical CSS, you can tell Craft Vite to not load your CSS asynchronously:

{{ craft.vite.script(PATH, false }}

See Other Options in the Vite documentation: https://nystudio107.com/docs/vite/#other-options

I'm assuming that this will fix the issue you're having in local dev as well?

cmalven commented 2 years ago

Thank you for the response, @khalwat

This isn't how Vite works in terms of separating the CSS from the JS.

This isn't accurate. Vite is geared primarily toward projects heavily using frontend Frameworks where you’re expected to “JS all the things”, so the docs don't discuss it in detail, but you can directly reference stylesheets in very similar ways to JS files instead of importing them in JS.

In a standard, non-Craft Vite project, you can do this either by linking to the file in your HTML directly (https://github.com/cmalven/vite-animation-boilerplate/blob/main/index.html#L10) or by adding it to rollupOptions.input in vite.config.js, just like your Craft Vite docs recommend doing for your main JS file:

// vite.config.js
build: {
  rollupOptions: {
    input: {
      app: './src/scripts/main.ts',
      styles: './src/styles/main.scss',
    },
  },
},

Not only can you reference styles in either of these ways, but they go through the same processing pipeline that they would if you imported them via JS. You can even apply Vite/Rollup plugins to them. It is 100% supported by Vite.

So you probably wouldn’t need to reference the manifest for these files - we can tell Vite directly that we’re using them - but we’d at least need a new Twig function that could intelligently load these style files based on the current environment in the same way that craft.vite.script does.

In any event, the FOUC is happening because by default, CSS is loaded async

I’m talking about local development, not production, and {{ craft.vite.script(PATH, false }} (in the <head>) doesn’t make a difference there because loading styles via JS doesn’t block rendering and is going to take long enough most of the time to cause at least some FOUC. The Javascript just can’t reliably be loaded, parsed, executed, and inject those styles into the page fast enough.

I just tested this locally with a JS file that does nothing but import a barebones SCSS file. Checking the Network tab in devtools, it takes around 7ms for the JS to load and then the styles are loaded around 8ms later. And this is for essentially an empty JS and SCSS file. Even using a CSS file instead of an SCSS file results in FOUC half of the time, and even a tiny amount of FOUC is enough to break styles that depend on being in place when the page is rendered.

As an aside… I should be able to add my SCSS file to rollupOptions.input and manually include <link rel="stylesheet" href="http://localhost:3000/src/styles/main.scss"> in my Twig, and things will kind of work, but there seems to be a bug in Craft Vite where if you don’t import at least one stylesheet in your main JS file, it won’t even connect to the dev server. Not sure why that would be.

It’s possible that no one else is bothered by this or would even use these additional features if they existed, and if that’s true I understand and feel free to disregard. But it is the only thing keeping me from adopting Craft Vite currently.

khalwat commented 2 years ago

Alright I will have a look to see if there isn't some way to accommodate what you're looking for. I wasn't aware that Vite will to the dev server interception of linked .css etc. files, but in retrospect, it makes complete sense that it would, and likely via the same mechanism that they are intercepted when imported via JS.

So I think we'd still need a manifest reference for these CSS files. That's because on production, they are going to be build with a content hash, so at least the fallback to production needs to extract them from the manifest just as it does now for JS files.

The additional wrinkle here is that unlike JS files, which are stored as top-level entries in the manifest, CSS files are not, or at least they aren't if they are imported via JS. I have to traverse the manifest looking for them via a recursive method: https://github.com/nystudio107/craft-plugin-vite/blob/develop/src/helpers/ManifestHelper.php#L216

I haven't tested it yet using the direct link method to a CSS file, who knows, maybe it will make them top-level manifest entries in that case.

Either way, I'm a little leery about behavior that works but isn't well documented/supported, but willing to give it a go! PRs are welcome too!

cmalven commented 2 years ago

@khalwat Thank you for being open to working on this. I think you're right that building output markup for production is the most challenging part, and Vite doesn't do much to help make that easy. I wish they'd do more to help backend-based implementations duplicate the same kind of output you'd get when building with a "pure Vite" project, but I understand that's not their focus.

Unfortunately, I don't think I understand the Vite internals or how Craft Vite works on a deep enough level to come up with a PR that wouldn't cause you to claw your eyes out, but I am more than happy to help stress test things if you arrive at something that you think is in a decent place.

Thanks, again.

khalwat commented 2 years ago

I think it won't be too bad, I just need to allocate some time, and then set up some text scenarios that you've outlined. Stay tuned.

Most of the code has already been written.

khalwat commented 2 years ago

So this actually isn't possible yet, because other resources are not currently included in the generated manifest.

This is a PR to add this to Vite 3 however:

https://github.com/vitejs/vite/pull/6649#issuecomment-1159278169

mike-moreau commented 2 years ago

Does the release of vite 3 make this a possibility?

khalwat commented 2 years ago

It should, yes @mikeatmostlys

plumduffer commented 2 years ago

I would also like to see this happen.

thupsi commented 2 years ago

+1 from me too!

juddlyon commented 2 years ago

+1 here too.

I'm working with CSS/JS provided by a third-party developer and if the CSS isn't included prior to the JS I get horrible FOUC issues.

juddlyon commented 2 years ago

In case anybody has a similar issue to me, I temporarily solved it by preventing asynchronous CSS and deferring the JS. Check the Other Options section of the Vite Plugin docs for all the options.

{{ craft.vite.script("../js/app.js", false, { 'defer': true }) }}

Still annoying in dev but fixed in production.

khalwat commented 2 years ago

The real pattern you should be using in production @juddlyon is to keep the stylesheet asynchronous, and use Critical CSS for the above the fold content.

juddlyon commented 2 years ago

Agreed, but I can never get it to bloody work without weird side-effects. Maybe I’ll try again with your Rollup plug-in.

Thanks for all your contributions @khalwat, you are a beast.

jrrdnx commented 1 year ago

+1 here as well

webrgp commented 1 year ago

@khalwat Big fan here and love the plugin! I think I found a simple solution that will need some minor change (of course, I may be completely off-base here)

I already have almost working on a existin project:

Setup:

In my vite.config.js I have:

import { resolve } from 'path';
import { normalizePath } from 'vite';
import { ViteFaviconsPlugin } from 'vite-plugin-favicon2';
import manifestSRI from 'vite-plugin-manifest-sri';
import legacy from '@vitejs/plugin-legacy';
import FullReload from 'vite-plugin-full-reload';

const serverUrl = process.env.DDEV_PRIMARY_URL || 'http://localhost';
const serverPort = parseInt(process.env.VITE_PRIMARY_PORT || '5173');

export default {
  base: command === 'serve' ? '' : '/dist/',
  publicDir: './src/public',
  build: {
    emptyOutDir: true,
    manifest: true,
    outDir: '../cms/web/dist',
    rollupOptions: {
      input: [
        './src/css/site.css',
        './src/js/site.js'
      ],
      output: {
        sourcemap: true,
      },
    },
  },
  plugins: [
    ...
  ],
  server: {
    fs: {
      strict: false,
    },
    origin: `${serverUrl}:${serverPort}`,
    host: '0.0.0.0',
    port: serverPort,
    strictPort: true,
  },
};

In my vite.php I have:

<?php

use craft\helpers\App;

return [
  'useDevServer' => App::env('ENVIRONMENT') === 'dev',
  'devServerPublic' => App::env('DEFAULT_SITE_URL') . ':' . App::env('VITE_PRIMARY_PORT'),
  'devServerInternal' => 'http://localhost:' . App::env('VITE_PRIMARY_PORT'),
  'manifestPath' => '@webroot/dist/manifest.json',
  'serverPublic' => '/dist/',
  'errorEntry' => 'src/js/site.js',
  'cacheKeySuffix' => '',
  'checkDevServer' => App::env('ENVIRONMENT') === 'dev',
  'includeReactRefreshShim' => false,
  'includeModulePreloadShim' => true,
  'criticalPath' => '@webroot/dist/criticalcss',
  'criticalSuffix' => '_critical.min.css',
];

With those settings in place, this is how I am loading styles and assets:

{# Styles in the <head> #}
{% set cssPath = 'src/css/site.scss' %}
<link rel="stylesheet" href="{{ craft.vite.asset(cssPath)|default(craft.vite.entry(cssPath)) }}">

{# Asset inside the publicDir in Vite  #}
{% set assetPath = '/asset.jpg' %}
<img src="{{ craft.vite.asset(assetPath)|default(craft.vite.entry(assetPath)) }}" />

{# Some third-party script inside the publicDir in Vite  #}
{% set libPath = '/lib.js' %}
<script src="{{ craft.vite.asset(libPath)|default(craft.vite.entry(libPath)) }}"></script>

When running vite the `craft.vite.asset() returns the correct path to the asset, but when vite build is run, the craft.vite.asset() returns an empty string. At the same time, craft.vite.entry() returns '/dist/' in development but the correct path to the asset after the build.

It seems to me craft.vite.asset() and craft.vite.entry() could be combined into a single function that would return the correct path to the asset in both development and production.

I am not sure if this is the best approach, but it seems to work for me. What do you think?

jrrdnx commented 1 year ago

@webrgp Thanks for posting this! Going off this, I was able to pretty much get to the point I was looking for as well. My vite.config.js and vite.php config files are essentially the same, but in my _wrapper template I'm using this in the <head>:

{% if not craft.vite.devServerRunning() %}
    <style title="criticalCss">
        {{ craft.vite.inline(craft.vite.entry('src/scss/critical.scss')) }}
    </style>
    <link rel="preload" as="style" href="{{ craft.vite.entry('src/scss/app.scss') }}" />
    <link rel="stylesheet" href="{{ craft.vite.entry('src/scss/app.scss') }}" media="print" onload="this.media='all'" />
    <noscript><link rel="stylesheet" href="{{ craft.vite.entry('src/scss/app.scss') }}" /></noscript>
{% endif %}

and this just above </body>:

{% if not craft.vite.devServerRunning() %}
    <script src="{{ craft.vite.entry('src/js/app.js') }}"></script>
{% else %}
    {{ craft.vite.script('src/js/app.js') }}
{% endif %}

We have several CSS files imported in the main app.js in order for HMR to work locally, but on production we use the workflow above so I wanted to get back to using that. Not sure if it's the best way to go about it, but it's working for now.

We do lose out on the module preload tags with this setup, but I'm not sure if there's a way to get those back while preventing craft.vite.script() from doubling the <link> tags.

The only thing we ran into was the craft.vite.inline() function needed the serverPublic setting to use the full URL, but I opened issue #55 for that.

khalwat commented 1 year ago

The best way to solve this will be using Vite 3.0.0 or later, and me making some additions to the Vite plugin to allow for the inclusion of CSS files in this manner.

I still recommend using the Critical CSS pattern mentioned above instead, but I'll make it work for those who for whatever reason don't want to do that.

khalwat commented 1 year ago

Relevant changes:

https://github.com/vitejs/vite/pull/6649

https://github.com/vitejs/vite/pull/8768

khalwat commented 1 year ago

Addressed this in a release:

Craft 3: Version 1.0.30 -> https://github.com/nystudio107/craft-vite/releases/tag/1.0.30

Craft 4: Version 4.0.5 -> https://github.com/nystudio107/craft-vite/releases/tag/4.0.5

Again, I'd still recommend you use a Critical CSS pattern, but if you really want to do this, and are using Vite 3.x or later, you can.

Relevant docs here: https://nystudio107.com/docs/vite/#using-craft-vite-asset-with-css

mike-moreau commented 1 year ago

Thank you @khalwat!

acalvino4 commented 1 year ago

I tried achieving something to this effect for css in my control panel, using an event like so (src/styles/cp.css is configured as an input source in vite.config.ts):

     Event::on(
            View::class,
            View::EVENT_BEFORE_RENDER_TEMPLATE,
            function(TemplateEvent $event) {
                Vite::getInstance()?->vite->register('src/styles/cp.css');
            }
        );

But that leads to a script tag being output still, as evidenced by this console message:

Failed to load module script: Expected a JavaScript module script but the server responded with a MIME type of "text/css". Strict MIME type checking is enforced for module scripts per HTML spec.

It seems this the solution to this issue was implemented via the assets function. Any chance we could get this to work with the register function as well? Or would the preferable solution be to use Craft::$app->getView()->registerCssFile() along with Vite::getInstance()?->vite->asset().

khalwat commented 1 year ago

@acalvino4 Can you open a new, separate Enhancement issue, and put in it the desire to have ->registerAsset() in addition to asset()?

acalvino4 commented 1 year ago

sure, thanks!