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

Simplify asset story #1438

Merged
merged 2 commits into from
May 2, 2024
Merged

Simplify asset story #1438

merged 2 commits into from
May 2, 2024

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented May 2, 2024

Horizon was migrated to Vite + laravel-vite-plugin. The laravel-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 the Vite 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.

# Build for production
npm run build

# Rebuild on changes
npm run watch

Minor changes

  • Build assets moved from /public/build to /dist

Considerations

  • This implementation doesn't currently support HMR. It can be easily added, but I'm not sure if we are developing Horizon enough to need it? Just run npm run watch and it will rebuild the assets whenever they change.

Copy link

github-actions bot commented May 2, 2024

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.

@timacdonald timacdonald force-pushed the assets branch 5 times, most recently from 31bcb72 to 2d7c6b6 Compare May 2, 2024 05:00
@timacdonald timacdonald changed the title wip Simplify asset story May 2, 2024
@timacdonald timacdonald force-pushed the assets branch 2 times, most recently from b4260c0 to aff2f8a Compare May 2, 2024 06:03
Comment on lines +3 to +34
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"
Copy link
Member Author

@timacdonald timacdonald May 2, 2024

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

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.

Comment on lines -11 to -12
import.meta.glob(['../img/**']);

Copy link
Member Author

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.

Comment on lines -130 to -135
@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

Copy link
Member Author

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

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.

@timacdonald timacdonald marked this pull request as ready for review May 2, 2024 06:11
@timacdonald
Copy link
Member Author

@driesvints, if you have a release script for this you will need to update the build command to npm run build.

@taylorotwell taylorotwell merged commit b867421 into laravel:5.x May 2, 2024
9 of 10 checks passed
@timacdonald timacdonald deleted the assets branch May 2, 2024 23:49
@driesvints
Copy link
Member

driesvints commented May 3, 2024

@timacdonald our repos with asset compilation use the compile assets workflow. I've made the necessary adjustments but just FYI it's this: 8d31ff1

@timacdonald
Copy link
Member Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants