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

Created vite() helper to replicate functionality of mix() helper #43098

Closed
wants to merge 1 commit into from

Conversation

chrispage1
Copy link
Contributor

For my project, I was using the mix() helper function in various areas to load the absolute path to compiled mix assets.

With the addition of Vite (which is awesome!), there is no such helper to retrieve absolute file paths, only strings bundled with style & script tags.

This helper allows a user to retrieve an absolute path to a Vite asset, either in hot reload or built asset mode.

Example:

@vite('resources/css/app.scss') results in a compiled HTML tag -

<link rel="stylesheet" href="http://localhost/build/assets/app.3c62ae02.css" />

whereas {{ vite('resources/css/app.scss') }} results in an absolute file path -

http://localhost/build/assets/app.3c62ae02.css

I hope you'll see this as a useful contribution to Laravel! I'd be happy to write up some documentation on this addition.

@ankurk91
Copy link
Contributor

ankurk91 commented Jul 8, 2022

Can you move this to a class, because there is no way to mock your helper during test.

@chrispage1 chrispage1 force-pushed the feature/vite-helper branch 2 times, most recently from fad411a to fc3db2a Compare July 8, 2022 10:31
@chrispage1
Copy link
Contributor Author

@ankurk91 I have refactored my work to make use of the Illuminate\Foundation\Vite class and provide some additional functionality / deduplication. Look forward to your thoughts.

I've also written a couple of tests for this.

@ankurk91
Copy link
Contributor

ankurk91 commented Jul 8, 2022

All looks good to me, since you are using existing class now.

One more feedback, Laravel team does not like the types, for example array|string, they prefer it in doc block only.
And yes they prefer FCQN in doc block.

Replaced nullsafe with isset check
Created vite() helper to replicate the functionality of the mix() helper
Abstracted vite() helper method into Vite foundation class
Added tests for generation of single URL path
@chrispage1 chrispage1 force-pushed the feature/vite-helper branch from fc3db2a to 8bed08c Compare July 8, 2022 11:12
@chrispage1
Copy link
Contributor Author

chrispage1 commented Jul 8, 2022

Thanks for the heads up @ankurk91 - I've reverted some of the refactors I've made :)

Out of interest, why is this? Would it not be good to enforce stricter coding standards?

Chris.

@ankurk91
Copy link
Contributor

ankurk91 commented Jul 8, 2022

Would it not be good to enforce stricter coding standards?

This is can be answered by Laravel team :)

@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

@chrispage1
Copy link
Contributor Author

@taylorotwell thanks for your message and completely understand. However I do feel this is a quality of life improvement for the framework that resolves an issue various people will face, including myself when upgrading to Vite.

In terms of addition to the code, there is very little baggage, if any and is mostly just refactors of existing code to enhance its usefulness. I hope you'll reconsider!

@chrispage1
Copy link
Contributor Author

For those interested, I have created a Vite helper plugin for the time being

composer require motomedialab/laravel-vite-helper

Hopefully this PR will be reconsidered one day :)

@tabuna
Copy link
Contributor

tabuna commented Jul 10, 2022

I faced the same issue #42785 (comment) and I think this is a very helpful change

@chrispage1
Copy link
Contributor Author

chrispage1 commented Jul 11, 2022

Thanks @tabuna . For now I hope the package is of use 👍

Chris.

@timacdonald
Copy link
Member

timacdonald commented Jul 12, 2022

Hey folks, just a heads up that all build mode generated URLs with the framework implementation are prefixed with the ASSET_URL env value. This is how Vite works on Vapor, for example.

So you can host your assets in S3 for production and link to the via an absolute URL as needed.

@chrispage1
Copy link
Contributor Author

Thanks @timacdonald although this issue solved the fact we don't know exactly what the compiled assets URL is going to be. The merge request and plugin I've made use the asset helper so won't have any problems dealing with assets on S3 etc

@chrispage1 chrispage1 deleted the feature/vite-helper branch March 6, 2023 16:56
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.

5 participants