wintercms / winter

Free, open-source, self-hosted CMS platform based on the Laravel PHP Framework.
https://wintercms.com
MIT License
1.36k stars 192 forks source link

Integrate Vite & Rework Mix #1141

Closed jaxwilko closed 2 months ago

jaxwilko commented 4 months ago

This PR implements support for Vite side by side with Mix. All exsiting Mix functionality has been retained, but all the commands have been abstracted to allow for re-use by Vite. This allows for full support of both compilers, at the same time.

Modified

New

Docs

Add a vite.config.js file to whatever package (plugin, theme, module) you want, then expect the same behaviour as you would with the mix:* commands, but with vite:* instead. This can be done for you via the vite:config command.

When using Vite, to load your assets in your markup you need to use the new vite() function. It takes 2 arguments, an array of entry points, and the package name for where the assets are loaded from.

This should end up looking like:

{{ vite(['assets/css/theme.css', 'assets/javascript/theme.js'], 'theme-example') }}

Note: The default vite port is 5173, you should ensure that port is open. If you are running multiple vite:watch commands at the same time, the port will increment with each watch, so open those ports too if required.

Vite can also be used in the backend:

<?= \System\Classes\Vite::tags(['assets/css/example-plugin.css'], 'example.plugin') ?>

TODO

bennothommo commented 4 months ago

Couple of initial comments:

jaxwilko commented 4 months ago

@bennothommo

  • Perhaps we could add :build and :dev command aliases to :compile and :watch commands, respectively. Both terms are more commonly used for JS libraries.

100% agree, makes sense.

  • If we're going to be separating the run and update commands under an npm namespace, then install should probably go under there too. Perhaps we should abstract that command to be able to define which path the user wants to go down (ie. install Mix or install Vite).

Install isn't just installing packages, it's validating the configs are set and adding packages to workspaces, then adding deps if required, I kinda like how it is atm, although maybe it should be named something different.

  • With the vite command in Twig, I would think we can determine the theme's base path automatically. Is there any specific reason why we need to define it explicitly?

Yes for theme, maybe for plugins, and maybe for somebody adding assets from a plugin to a theme (idk why but they could). All these edge cases are solved by just setting the same base that we pass when executing the command. Basically the php side needs to match up with the js web socket server. We could default to a guess, but it may cause more confusion when it fails than just setting a path and being done with it.

bennothommo commented 4 months ago

Thanks for the feedback @jaxwilko.

Install isn't just installing packages, it's validating the configs are set and adding packages to workspaces, then adding deps if required, I kinda like how it is atm, although maybe it should be named something different.

That's cool by me. I just would like a rational and natural way for someone to "update" their dependencies - especially with the frequency of JS libraries and their dependencies being affected by security vulnerabilities. I suppose that npm:update would work - perhaps allowing us to reserve vite:upgrade for doing more thorough upgrades to Vite if, say, down the track we need to make configuration changes as part of an upgrade process (ie. allowing the command to do major upgrades to Vite).

Yes for theme, maybe for plugins, and maybe for somebody adding assets from a plugin to a theme (idk why but they could). All these edge cases are solved by just setting the same base that we pass when executing the command. Basically the php side needs to match up with the js web socket server. We could default to a guess, but it may cause more confusion when it fails than just setting a path and being done with it.

I've been thinking about this exact scenario for some time - pretty much since we introduced Mix. I think the easiest way we can move forward on this is that we have to consider that each plugin and theme is responsible for its own build and assets and that's it. The fact is, not everyone is going to write plugins and components that work nicely with every type of build used by themes, especially when our goal with the CMS side is that "people can use whatever front-end framework they want".

As you said too, people can have the plugins (or at the very least, the components) inject assets into the theme when used. I still would argue that it would be most likely that the "entrypoint" to these assets would still reside in the theme if they built the plugins themselves. For third-party plugins, it's fair game for them to have their own build process and you simply use the compiled assets (or perhaps the build process in the theme could concatenate/minify these assets into the theme build). Either way, I think they're both out of scope for the vite filter you would be introducing.

So I wonder if we can just simplify the vite filter to assume all paths are relative to the theme, and make it so that the entrypoints always exist within the theme (these entrypoints can however import plugin assets however they wish).

Thoughts on this?

jaxwilko commented 4 months ago

I don't think vite will play ball, unless we only enable vite for themes and not plugins (which was my first thought before I was able to get plugins working too).

By all means have a play with it, but I think currently just adding the base covers all possible situations with the most flexibility.

jaxwilko commented 4 months ago

@LukeTowers @bennothommo the param ordering for getPackages() has been swapped. I've also added some extra helpers to CompilableAssets & PackageJson to simplify logic in some commands.

LukeTowers commented 4 months ago

@mjauvin do you feel like having a play around with this to help test it?

mjauvin commented 4 months ago

You all know the love I have for npm... why do you even ask? 😂

jaxwilko commented 3 months ago

@bennothommo had a thought, what if we swap the "base path thing" for the package name, i.e. theme-example or winter.test, then resolve the base via CompilableAssets.

That way it makes a lot more sense as you're "loading entry files from this package".

Any thoughts?

bennothommo commented 3 months ago

That sounds a lot better. I still think it can default to the theme base path, but yeah having it allow the theme and plugin codes that Mix uses would give that extra flexibility in a simple way. 👍

LukeTowers commented 3 months ago

You all know the love I have for npm... why do you even ask? 😂

That's why I ask 😂

jaxwilko commented 3 months ago

I've put together a simple testing guide to try out vite + the new config command.

Set up a testing plugin, configure it and add stubs:

./artisan create:plugin Example.Test
./artisan create:component Example.Test Testing
./artisan vite:config Example.Test --tailwind --stubs
./artisan vite:install

echo  -e "<p class=\"text-red-900\">hello world</p>\n{{ vite(['assets/css/example-test.css'], 'example.test') }}" > plugins/example/test/components/testing/default.htm

Enable the component in the plugin:

return [
    \Example\Test\Components\Testing::class => 'testing',
];

Add the component to one of your pages:

...
[testing]
...
==
...
{% component testing %}
...

Run vite:

./artisan vite:watch example.test
jaxwilko commented 3 months ago

I've added a helper so you can run vite in the backend too.

<?= \System\Classes\Vite::tags(['assets/css/example-plugin.css'], 'example.plugin') ?>
LukeTowers commented 3 months ago

@jaxwilko does it make sense to add support to the AssetMaker trait as well?

jaxwilko commented 3 months ago

Relies on: https://github.com/wintercms/storm/pull/183

jaxwilko commented 2 months ago

I've tested this with an existing theme in a completed project, it functioned as expected, however as that project uses https by default it caused some mixed content issues. This could be resolved by switching back to http only or via a fix from here: https://stackoverflow.com/questions/69417788/vite-https-on-localhost or sticking vite behind nginx. Either way it falls outside the scope of this PR but will be necessary to document.

bennothommo commented 2 months ago

@jaxwilko I've given this a test - it's really cool, and worked nicely as far as I could tell.

However, if I am to be brutally honest, I still prefer the configuration API of Mix. It's an API that someone who detests JavaScript (ie. @LukeTowers :stuck_out_tongue:) would still feel comfortable using. Vite is obviously leaps and bounds more easy to wrangle than Webpack, but it's still very declarative for all but the most simple of cases (someone using the base config and simply using the vite() method in their templates).

As soon as someone needs to do something complex, you're back into defining huge config and plugin objects in the config file. There's no way (I think) that I can do my Monaco Editor build with Vite currently.

I'm not saying we should do this now given that your intention is to still allow Mix to work, but since the underlying laravel-mix library is all but abandoned at this point, I'm thinking that it would be nice down the track to create a wrapper to configure Vite in the same way Mix did for Webpack, if that's at all feasible.

jaxwilko commented 2 months ago

@bennothommo

I still prefer the configuration API of Mix. It's an API that someone who detests JavaScript (ie. @LukeTowers 😛)

The beauty of Winter is we can support both (and with all the abstraction I added, we can now easily support FrameworkX+n when it becomes the default next month). If I'm honest I used to write Webpack configs before writing that mix plugin years back so either mix or vite configs don't bother me much :joy:.

I'm thinking that it would be nice down the track to create a wrapper to configure Vite in the same way Mix did for Webpack

Rollup vs. Webpack are very different, the way they opperate is different and it comes with some advantages and some disadvantages.

One issue I found earlier is that if you're running a site in https, then vite will cause issues as it's webserver is http by default, you need to go out of your way to either wrap it under nginx or use one of the many plugins that exist to fix the issue for you (or manually configure vite with your own certs).

I'm sure there are many more similar issues due to the differing paradigms between Mix & Vite, which is why I want to offer support for allowing plugins, themes and registerable things (i.e. direct calls to CompilableAssets) to manage a vite/mix "package", but kind of be hands off at that point, allow for whatever configuration the user wants to do with vite/mix but not get in their way.

The way I see it, if they have an issue with vite, they will google "Vite not work with this thing", which will probably get a result that tells them what to put in their config to fix it. If we change that to "winter-vite-wrapper thing not work" then they will likely get far less results and the issue will fall on us to support our custom extension.

but since the underlying laravel-mix library is all but abandoned at this point

Till it starts causing issues I'd like to keep supporting it, I think Vite will work very well for frontend themes but I'm not sure how well it'll work in the backend and I think Mix may generally work better due to it's simplicity (that said, @LukeTowers made be add a AssetMaker::addVite() method which does work nicely). We're still supporting Assetic after all.

As soon as someone needs to do something complex, you're back into defining huge config and plugin objects in the config file. There's no way (I think) that I can do my Monaco Editor build with Vite currently.

Ping me when you're next free on discord and we can try and get it working :). I've tried out 2 production sites converting existing mix configs over but they weren't doing much clever stuff so it would be a good test to find any edge cases.

LukeTowers commented 2 months ago

@jaxwilko is there a docs PR?

jaxwilko commented 2 months ago

@LukeTowers working on it atm :)

jaxwilko commented 2 months ago

Docs PR: https://github.com/wintercms/docs/pull/200

LukeTowers commented 2 months ago

Just waiting for @bennothommo to weigh in on the remaining two items (package.json being excluded from the source export & moving the PackageJson class into either the Storm Parse package or the Laravel Config Writer package).

LukeTowers commented 2 months ago

@jaxwilko @bennothommo are there any remaining items before we can merge this?

bennothommo commented 2 months ago

Not that I'm aware of.