Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate Vite & Rework Mix #1141

Merged
merged 63 commits into from
Jul 16, 2024
Merged

Integrate Vite & Rework Mix #1141

merged 63 commits into from
Jul 16, 2024

Conversation

jaxwilko
Copy link
Member

@jaxwilko jaxwilko commented Jun 5, 2024

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

  • Commands
    • mix:compile, mix:watch, mix:install, mix:list all now rely on abstract Asset* commands and inherit 90% of the functionality to allow for Vite* commands to reuse the same code.
    • mix:run & mix:update - these were used for interacting with npm via the Winter CLI, these have been renamed to refelect their function to npm:run & npm:update. Alias have been set to refer back to their previous names incase anybody is using them in scripts.

New

  • Commands
    • vite:compile - shares args with mix:compile called vite build under the hood
    • vite:watch - shares args with mix:watch called vite under the hood (defaults to "watching")
    • vite:install - shares args with mix:install allows for configuring package.json workspaces and installing required global packages
    • vite:list - shares args with mix:list
    • vite:config - new allows for setting up stub files
    • mix:config - new allows for setting up stub files
  • System\Classes\PackageJson - Now provides a consistant interface for modifying package.json files rather than using json_encode/json_decode everywhere
  • Twig function vite() has been added, this works similarly to Laravel's @vite() in blade, however it required the base path to also be passed.
  • vite:watch, vite:compile, mix:watch & mix:compile now support --disable-tty which passes output back to the Symfony Process allowing for capture by callers (useful in testing).

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

  • Testing
    • Fresh install
    • Existing install with mix
    • Fresh plugin
    • Fresh theme
    • Linux
    • Mac
    • Windows
  • Vite TestCases
  • *:config TestCases

modules/system/classes/CompilableAssets.php Outdated Show resolved Hide resolved
modules/system/classes/CompilableAssets.php Outdated Show resolved Hide resolved
modules/system/ServiceProvider.php Outdated Show resolved Hide resolved
modules/cms/twig/Extension.php Outdated Show resolved Hide resolved
@bennothommo
Copy link
Member

Couple of initial comments:

  • Perhaps we could add *:build and *:dev command aliases to *:compile and *:watch commands, respectively. Both terms are more commonly used for JS libraries.
  • 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).
  • 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?

@jaxwilko
Copy link
Member Author

jaxwilko commented Jun 5, 2024

@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
Copy link
Member

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
Copy link
Member Author

jaxwilko commented Jun 6, 2024

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.

@LukeTowers LukeTowers changed the title [WIP] Intergrate Vite & Rework Mix [WIP] Integrate Vite & Rework Mix Jun 6, 2024
@LukeTowers LukeTowers added this to the 1.3.0 milestone Jun 6, 2024
modules/system/classes/CompilableAssets.php Outdated Show resolved Hide resolved
modules/system/classes/CompilableAssets.php Outdated Show resolved Hide resolved
modules/system/classes/CompilableAssets.php Outdated Show resolved Hide resolved
modules/system/classes/CompilableAssets.php Outdated Show resolved Hide resolved
@jaxwilko
Copy link
Member Author

Docs PR: wintercms/docs#200

modules/cms/twig/Extension.php Outdated Show resolved Hide resolved
modules/system/classes/CompilableAssets.php Outdated Show resolved Hide resolved
modules/system/classes/PackageJson.php Outdated Show resolved Hide resolved
modules/system/classes/PackageJson.php Show resolved Hide resolved
modules/system/classes/Vite.php Outdated Show resolved Hide resolved
modules/system/console/asset/AssetConfig.php Show resolved Hide resolved
modules/system/console/asset/AssetInstall.php Outdated Show resolved Hide resolved
modules/system/traits/AssetMaker.php Outdated Show resolved Hide resolved
modules/system/classes/CompilableAssets.php Outdated Show resolved Hide resolved
@jaxwilko jaxwilko requested a review from LukeTowers July 14, 2024 21:25
@LukeTowers
Copy link
Member

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
Copy link
Member

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

@bennothommo
Copy link
Member

Not that I'm aware of.

@LukeTowers LukeTowers changed the title [WIP] Integrate Vite & Rework Mix Integrate Vite & Rework Mix Jul 16, 2024
@LukeTowers LukeTowers modified the milestones: 1.3.0, 1.2.7 Jul 16, 2024
@LukeTowers LukeTowers merged commit 21b6f4d into develop Jul 16, 2024
11 checks passed
@LukeTowers LukeTowers deleted the wip/vite-mix-v2 branch July 16, 2024 05:44
LukeTowers pushed a commit to wintercms/docs that referenced this pull request Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants