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

Add suport to Vite #1308

Merged
merged 13 commits into from
Oct 5, 2024
Merged

Add suport to Vite #1308

merged 13 commits into from
Oct 5, 2024

Conversation

ruanpepe
Copy link
Contributor

@ruanpepe ruanpepe commented Sep 20, 2024

Question Answer
Pull request type ENHANCEMENT
License MIT

What's in this PR?

In config:
I changed the Laravel Mix config lines to a new patter that can be used with Vite
In that new way, the user can choose between 'mix', 'vite' and false.

In master.blade.php:
I included a switch to include the css and js files according to the config.
It is checking if the user is using the old mix config, so it is not a breaking change.

In components
I included a check to prevent scripts from running before jQuery loading finish.

The wiki must be updated
https://github.com/jeroennoten/Laravel-AdminLTE/wiki/Menu-Configuration

My tests included

  • Using Mix
  • Using Vite

Further tests are recommended

Checklist

  • I tested these changes.
  • I have linked the related issues.

@ruanpepe
Copy link
Contributor Author

Related issue
#1155

@dfsmania dfsmania marked this pull request as draft September 20, 2024 20:38
@dfsmania dfsmania self-requested a review September 20, 2024 20:38
@dfsmania
Copy link
Collaborator

Thanks for your contribution @ruanpepe , give me some time to review this, could you provide further information about next changes:

In components
I included a check to prevent scripts from running before jQuery loading finish.

My doubt is why the already included shorthand check for $( document ).ready() of JQuery won't work for the VITE support you included.

resources/views/master.blade.php Outdated Show resolved Hide resolved
@ruanpepe
Copy link
Contributor Author

ruanpepe commented Sep 22, 2024

Thanks for your contribution @ruanpepe , give me some time to review this, could you provide further information about next changes:

In components
I included a check to prevent scripts from running before jQuery loading finish.

My doubt is why the already included shorthand check for $( document ).ready() of JQuery won't work for the VITE support you included.

jQuery is included in my app.js and Vite loads the js files asynchronously. I have tried the $( document ).ready() but i had a lot of work solving "$ is not a function".
I placed some logs on my app.js and in the page.blade.php js section. The Vite tag was the first in the code, but the log in the app.js was the last in the console.
The components were running before Vite finish loading my app.js.
I noticed that after see my app.js log on console, the $ could be normally called.

@dfsmania
Copy link
Collaborator

Thanks for your contribution @ruanpepe , give me some time to review this, could you provide further information about next changes:

In components
I included a check to prevent scripts from running before jQuery loading finish.

My doubt is why the already included shorthand check for $( document ).ready() of JQuery won't work for the VITE support you included.

jQuery is included in my app.js and Vite loads the js files asynchronously. I have tried the $( document ).ready() but i had a lot of work solving "$ is not a function". I placed some logs on my app.js and in the page.blade.php js section. The Vite tag was the first in the code, but the log in the app.js was the last in the console. The components were running before Vite finish loading my app.js. I noticed that after see my app.js log on console, the $ could be normally called.

Oh, I see. I have read next link about the problem you faced:

https://laracasts.com/discuss/channels/vite/laravel-vite-jquery

Can you try the fix proposed by user: hassan, it requires to add type="module" to the script tags instead of wrapping all the code in document.addEventListener('DOMContentLoaded', ...);. If this workaround works, I would prefer it since it requires less changes on the base code of the componets...

@ruanpepe
Copy link
Contributor Author

ruanpepe commented Sep 23, 2024

Thanks for your contribution @ruanpepe , give me some time to review this, could you provide further information about next changes:

In components
I included a check to prevent scripts from running before jQuery loading finish.

My doubt is why the already included shorthand check for $( document ).ready() of JQuery won't work for the VITE support you included.

jQuery is included in my app.js and Vite loads the js files asynchronously. I have tried the $( document ).ready() but i had a lot of work solving "$ is not a function". I placed some logs on my app.js and in the page.blade.php js section. The Vite tag was the first in the code, but the log in the app.js was the last in the console. The components were running before Vite finish loading my app.js. I noticed that after see my app.js log on console, the $ could be normally called.

Oh, I see. I have read next link about the problem you faced:

https://laracasts.com/discuss/channels/vite/laravel-vite-jquery

Can you try the fix proposed by user: hassan, it requires to add type="module" to the script tags instead of wrapping all the code in document.addEventListener('DOMContentLoaded', ...);. If this workaround works, I would prefer it since it requires less changes on the base code of the componets...

Using 'vite' and 'mix', it woks. Using 'vite_spa', the css is being loaded after js. Because it, the raw content of page becomes visible for a short moment.
I just removed this option from that code i wrote.

@dfsmania
Copy link
Collaborator

@ruanpepe OK, the problem you faced with your new vite_spa option is called flash of unstyled content. I will check if there's a fix proposed for this problem. On the other hand, could you rollback again the indent of the master.blade.php file.

In the next days, I will give these changes a test on some of my projects, just to check everything still works. Note I do not use Mix or Vite, so testing with Mix and Vite is up to you.

Finally, thanks for all your work here!

@ruanpepe
Copy link
Contributor Author

@ruanpepe OK, the problem you faced with your new vite_spa option is called flash of unstyled content. I will check if there's a fix proposed for this problem. On the other hand, could you rollback again the indent of the master.blade.php file.

In the next days, I will give these changes a test on some of my projects, just to check everything still works. Note I do not use Mix or Vite, so testing with Mix and Vite is up to you.

Finally, thanks for all your work here!

Great!
I fixed the ident. My IDE does it. Sorry!
Now, knowing what is the CSS issue, i can get better results looking for a solution.

It's a great pleasure contribute to this project i use for so long and for free. And it's a great experience to be in touch with another dev in another language. Thank you.

@ruanpepe
Copy link
Contributor Author

@ruanpepe OK, the problem you faced with your new vite_spa option is called flash of unstyled content. I will check if there's a fix proposed for this problem. On the other hand, could you rollback again the indent of the master.blade.php file.

In the next days, I will give these changes a test on some of my projects, just to check everything still works. Note I do not use Mix or Vite, so testing with Mix and Vite is up to you.

Finally, thanks for all your work here!

I did some research about flashing unstyled content in Vite, and after some testing using 'vite_spa' I noticed that this only happens in dev mode (npm run dev). In prod (npm run build) it is not an issue.

@dfsmania
Copy link
Collaborator

@ruanpepe Good, you can add that option again if you like, and then we may add a warning on the Wiki for the flash of unstyled content issue when working in dev mode. However I'm not sure if vite_spa is the better name for it, since SPA means Single Page Application, so maybe vite_js_only is more meaningful. Laravel says next for this option:

If you are building an SPA, including applications built using Inertia, Vite works best without CSS entry points.
Instead, you should import your CSS via JavaScript. Typically, this would be done in your application's resources/js/app.js.

@ruanpepe
Copy link
Contributor Author

ruanpepe commented Sep 24, 2024

Nice!
I'm goig to do it right now.

@dfsmania
Copy link
Collaborator

@ruanpepe I reverted some style fixes added by your editor, updated the config docs, and joined vite and vite_js_only into the same case block for the scripts section. So, I will give a test to these changes locally in the next days. Please, check I didn't break anything, just in case.

@ruanpepe
Copy link
Contributor Author

Everything is ok

@dfsmania
Copy link
Collaborator

@ruanpepe Great, I have update Wiki docs to address these changes, please review it, any suggestion would be accepted.

https://github.com/jeroennoten/Laravel-AdminLTE/wiki/Other-Configuration

@dfsmania dfsmania marked this pull request as ready for review September 25, 2024 23:09
@ruanpepe
Copy link
Contributor Author

ruanpepe commented Sep 30, 2024

@ruanpepe Great, I have update Wiki docs to address these changes, please review it, any suggestion would be accepted.

https://github.com/jeroennoten/Laravel-AdminLTE/wiki/Other-Configuration

There are two Laravel Mix sections (1, 2). I think the second one would be Laravel Vite, right?
Other than that, everything is very clear.

@dfsmania
Copy link
Collaborator

@ruanpepe Great, I have update Wiki docs to address these changes, please review it, any suggestion would be accepted.
https://github.com/jeroennoten/Laravel-AdminLTE/wiki/Other-Configuration

There are two Laravel Mix sections (1, 2). I think the second one would be Laravel Vite, right? Other than that, everything is very clear.

Thanks for pointing this! Fixed now!

@dfsmania dfsmania merged commit 9282c7b into jeroennoten:master Oct 5, 2024
34 checks passed
@dfsmania
Copy link
Collaborator

Thanks for your contribution @ruanpepe , give me some time to review this, could you provide further information about next changes:

In components
I included a check to prevent scripts from running before jQuery loading finish.

My doubt is why the already included shorthand check for $( document ).ready() of JQuery won't work for the VITE support you included.

jQuery is included in my app.js and Vite loads the js files asynchronously. I have tried the $( document ).ready() but i had a lot of work solving "$ is not a function". I placed some logs on my app.js and in the page.blade.php js section. The Vite tag was the first in the code, but the log in the app.js was the last in the console. The components were running before Vite finish loading my app.js. I noticed that after see my app.js log on console, the $ could be normally called.

Oh, I see. I have read next link about the problem you faced:

https://laracasts.com/discuss/channels/vite/laravel-vite-jquery

Can you try the fix proposed by user: hassan, it requires to add type="module" to the script tags instead of wrapping all the code in document.addEventListener('DOMContentLoaded', ...);. If this workaround works, I would prefer it since it requires less changes on the base code of the componets...

@ruanpepe The type="module" introduced some sort of backward incompatibility, I notice this in one of the projects I'm working on. Let me explain:

When you submit a form, most of the form components have a enable-old-support property that is used to auto setup the old() value submitted within the form when there is a validation error. Some of these components that are based in plugins, like the SelectBs use JQuery or Javascript to perform this task. Now, suppose I already have a view with a Javascript code to inspect the value of a SelectBs element after a form is submitted in order to enable/disable others elements based on its value. The problem I'm facing is that the setup of the old value of the SelectBs is running after my code, so the value I read is actually not the one shown in the form.

I know I can fix this by adding type="module" to all my extra Javascript/Jquery code pushed, but I think we should find another solution to address this backward compatibility. As a side note, using the document.addEventListener("DOMContentLoaded", function() {...} workaround also introduced this backward compatibility problem.

@ruanpepe
Copy link
Contributor Author

I hadn't noticed this behavior.
My first suggestion is stop using type="module" for while and remove vite_js_only option support.
I have no other quick fix right now.

@dfsmania
Copy link
Collaborator

dfsmania commented Oct 15, 2024

@ruanpepe Ok, I will try to investigate further. Anyway, doesn't Vite always load the app.js asynchronously (i.e. is it matter whether using vite_js_only or just vite)?

Maybe other solution may be to include JQuery using the plugins config when using Vite, instead of bundling it in app.js. Can you check if this work??

Also, can you try solutions that require extra configuration with VITE, for example:
https://www.mapledesign.co.uk/tech-blog/vite-bootstrap-4-jquery/
vitejs/vite#3744

My ideal goal would be to remove the added type="module" from the components and provide some extra example of configuration that will be required in VITE when using it here.

@ruanpepe
Copy link
Contributor Author

@ruanpepe Ok, I will try to investigate further. Anyway, doesn't Vite always load the app.js asynchronously (i.e. is it matter whether using vite_js_only or just vite)?

Maybe other solution may be to include JQuery using the plugins config when using Vite, instead of bundling it in app.js. Can you check if this work??

Also, can you try solutions that require extra configuration with VITE, for example:
https://www.mapledesign.co.uk/tech-blog/vite-bootstrap-4-jquery/
vitejs/vite#3744

My ideal goal would be to remove the added type="module" from the components and provide some extra example of configuration that will be required in VITE when using it here.

I'm going to test it, and tomorrow i will let you know the results.

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.

2 participants