-
Notifications
You must be signed in to change notification settings - Fork 645
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
Simplify asset story #1438
Simplify asset story #1438
Conversation
Thanks for submitting a PR! Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
31bcb72
to
2d7c6b6
Compare
b4260c0
to
aff2f8a
Compare
js: | ||
tab-width: 4 | ||
use-tabs: false | ||
print-width: 120 | ||
double-quotes: false | ||
trailing-commas: es5 | ||
semicolons: true | ||
arrow-parens: always | ||
bracket-same-line: false | ||
bracket-spacing: true | ||
finder: | ||
exclude: | ||
- "dist" | ||
- "node_modules" | ||
- "vendor" | ||
name: | ||
- "*.js" | ||
- "*.jsx" | ||
css: | ||
tab-width: 4 | ||
use-tabs: false | ||
print-width: 120 | ||
double-quotes: false | ||
finder: | ||
exclude: | ||
- "dist" | ||
- "node_modules" | ||
- "vendor" | ||
name: | ||
- "*.css" | ||
- "*.scss" | ||
- "*.less" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StyleCI was having issues with the files in dist
.
I've copied this config from their docs (this is what is configured with js: true
or css: true
and added dist
to the exclude.
"md5": "^2.3.0", | ||
"moment": "^2.30.1", | ||
"moment-timezone": "^0.5.45", | ||
"phpunserialize": "^1.3.0", | ||
"resolve-url-loader": "^5.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a webpack plugin. We don't need it anymore as we use Vite.
import.meta.glob(['../img/**']); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was here for the Favicon. We don't need it anymore as we inline the favicon.
@if (! $assetsAreCurrent) | ||
<div class="alert alert-warning"> | ||
The published Horizon assets are not up-to-date with the installed version. To update, run:<br/><code>php artisan horizon:publish</code> | ||
</div> | ||
@endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assets are always up to date ✌️
], | ||
resolve: { | ||
alias: { | ||
vue: "vue/dist/vue.esm.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this alias and used it inline instead to make it more explicit. This was kind of hidden otherwise.
@driesvints, if you have a release script for this you will need to update the build command to |
@timacdonald our repos with asset compilation use the compile assets workflow. I've made the necessary adjustments but just FYI it's this: 8d31ff1 |
Thanks! |
Horizon was migrated to Vite +
laravel-vite-plugin
. Thelaravel-vite-plugin
is not meant for packages. There was some additional work done to support the asset URL. We ended up having a lot of workarounds to make things work.This PR rethinks Horizon's asset story completely to try and simplify things.
Main changes
Inline assets on the dashboard
CSS, JS, and the favicon are now delivered inline. The horizon dashboard is a single-page app, so there is no real-world drawbacks here IMO.
The total transfer size is ~280kb over the wire. Yes we lose browser caching of the assets, but we also had to introduce a lot of manual work to add cache busting and 280kb is nothing.
This just works™️, assets are always up to date, we no longer have to publish assets whenever we change them, and applications that use a CDN for assets are no longer having to deliver assets via their web servers - which was an issue that has been raised.
Remove
laravel-vite-plugin
and usages of theVite
facade.As I mentioned, the
laravel-vite-plugin
is meant for Laravel apps. It doesn't really offer anything for packages and in some ways makes things harder.We just use raw Vite now when working on Horizon.
Minor changes
/public/build
to/dist
Considerations
npm run watch
and it will rebuild the assets whenever they change.