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

Feature/profiler #26

Merged
merged 30 commits into from
Aug 20, 2024
Merged

Feature/profiler #26

merged 30 commits into from
Aug 20, 2024

Conversation

lhapaipai
Copy link
Owner

@lhapaipai lhapaipai commented Apr 21, 2024

  • feature
  • update recipe
  • tests

Migration guide:

Remove the ./config/routes/dev/pentatrion_vite.yaml file

add the ./config/routes/pentatrion_vite.yaml file :

when@dev:
    _pentatrion_vite:
        prefix: /build
        resource: "@PentatrionViteBundle/Resources/config/routing.yaml"

    _profiler_vite:
        path: /_profiler/vite
        defaults:
            _controller: Pentatrion\ViteBundle\Controller\ProfilerController::info

Copy link

netlify bot commented Apr 21, 2024

Deploy Preview for cosmic-bubblegum-aa631a ready!

Name Link
🔨 Latest commit 98cb06b
🔍 Latest deploy log https://app.netlify.com/sites/cosmic-bubblegum-aa631a/deploys/66bcf25d438ee80009ae50eb
😎 Deploy Preview https://deploy-preview-26--cosmic-bubblegum-aa631a.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@andyexeter andyexeter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor suggestions :)

Also looks like there are a few CI failures.

src/vite-bundle/src/Service/Debug.php Outdated Show resolved Hide resolved
src/vite-bundle/src/Twig/TypeExtension.php Outdated Show resolved Hide resolved
src/vite-bundle/src/Service/Debug.php Show resolved Hide resolved
src/vite-bundle/src/Service/Debug.php Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
@lhapaipai
Copy link
Owner Author

lhapaipai commented Apr 29, 2024

hi @andyexeter,
thank you for your review, of course the functionality is in wip, I can't take the time to finish it at the moment, I opened this pr to see more easily what remained to be done. I will keep you informed when the functionality is ready (having corrected following your return)
Have a nice day !

@lhapaipai
Copy link
Owner Author

lhapaipai commented May 8, 2024

Hi @andyexeter, for the profiler I need to update the vite-bundle flex recipe : https://github.com/lhapaipai/recipes-contrib/tree/update/vite-bundle-7 Is there anything you would like to change in the current recipe?

@lhapaipai lhapaipai requested a review from andyexeter May 9, 2024 09:05
@lhapaipai
Copy link
Owner Author

lhapaipai commented May 9, 2024

I @andyexeter, if you have time to review the feature, you're welcome :-), else no problem.

I'm not too sure how to push the functionality into production.

updating without applying the patch:

when@dev:
     _pentatrion_vite:
         prefix: /build
         resource: "@PentatrionViteBundle/Resources/config/routing.yaml"

     _profiler_vite:
         path: /_profiler/vite
         defaults:
             _controller: Pentatrion\ViteBundle\Controller\ProfilerController::info

crashes the debug bar which causes a not very pleasant developer experience but at the same time that's what major updates are for.

so I don't know if I should wait for the recipe to be merged before activating the functionality in the debug bar

{# src/vite-bundle/src/Resources/views/Collector/vite_collector.html.twig #}
{% extends "@WebProfiler/Profiler/layout.html.twig" %}

{% block toolbar %}
    {% set icon %}
    {{ include('@PentatrionVite/Collector/icon.svg') }}
    <span class="sf-toolbar-value">Vite</span>
    {% endset %}
    {% set text %}
-    <div class="sf-toolbar-info-piece">
-        <b>Vite dev Server</b>
-        <span>
-        <a href="{{ path('_profiler_vite') }}">Config</a>
-        </span>
-    </div>
    <div class="sf-toolbar-info-piece">
        <b>Rendered &lt;script /&gt;</b>
        <span class="sf-toolbar-status">{{ collector.renderedScripts | length }}</span>
    </div>
    <div class="sf-toolbar-info-piece">
        <b>Rendered &lt;link /&gt;</b>
        <span class="sf-toolbar-status">{{ collector.renderedStyles | length }}</span>
    </div>
    {% endset %}
    {{ include('@WebProfiler/Profiler/toolbar_item.html.twig', { 'link': true }) }}
{% endblock %}

what do you think ?

EDIT:
finally I updated the recipe to be compatible with v6.4.7
so after your feedback on the modification of the recipe I will create the pr for the recipes-contrib repository and only when it is merged we will upgrade symfony-vite to v7

@andyexeter
Copy link
Collaborator

Hi @lhapaipai I have been away on holiday last week. I will try to look at this this week :)

@lhapaipai
Copy link
Owner Author

no problem, we do this when we have time, there is no rush 😄

@lhapaipai
Copy link
Owner Author

Hi @andyexeter,
I think I'll merge this pr, but let me know if you still want to do a review

@andyexeter
Copy link
Collaborator

Hi @lhapaipai,

This looks good to me.

Only thing I'd like to query is whether the profiler can be added to Symfony's profiler instead: https://symfony.com/doc/current/profiler.html#profiler-data-collector

That way, we wouldn't need our own controller or route and wouldn't need people to modify their routing config to access the profiler.

@lhapaipai
Copy link
Owner Author

lhapaipai commented Aug 15, 2024

hi @andyexeter
as you can see
debug

symfony console debug:router
 ----------------------------- -------- -------- ------ ----------------------------------- 
  Name                          Method   Scheme   Host   Path                               
 ----------------------------- -------- -------- ------ ----------------------------------- 
  _preview_error                ANY      ANY      ANY    /_error/{code}.{_format}           
  pentatrion_vite_build_proxy   ANY      ANY      ANY    /build/{path}                      
  _profiler_vite                ANY      ANY      ANY    /_profiler/vite                    
  _wdt                          ANY      ANY      ANY    /_wdt/{token}                      
  _profiler_home                ANY      ANY      ANY    /_profiler/                        
  _profiler_search              ANY      ANY      ANY    /_profiler/search                  
  _profiler_search_bar          ANY      ANY      ANY    /_profiler/search_bar              
  _profiler_phpinfo             ANY      ANY      ANY    /_profiler/phpinfo                 
  _profiler_xdebug              ANY      ANY      ANY    /_profiler/xdebug                  
  _profiler_font                ANY      ANY      ANY    /_profiler/font/{fontName}.woff2   
  _profiler_search_results      ANY      ANY      ANY    /_profiler/{token}/search/results  
  _profiler_open_file           ANY      ANY      ANY    /_profiler/open                    
  _profiler                     ANY      ANY      ANY    /_profiler/{token}                 
  _profiler_router              ANY      ANY      ANY    /_profiler/{token}/router          
  _profiler_exception           ANY      ANY      ANY    /_profiler/{token}/exception       
  _profiler_exception_css       ANY      ANY      ANY    /_profiler/{token}/exception.css   
  welcome                       ANY      ANY      ANY    /         

the screenshot is associated to _profiler -> /_profiler/{token} with the data of the request with token id. when we query /_profiler/phpinfo or /_profiler/xdebug or /_profiler/vite we have data associated with the current state of our system not the current query wit {token} id.
by integrating the profiler info as is in the vite panel of a query I fear that there will be confusion and that the developer will believe that it was the state of the vite server at the time of the query.

use case

  • we consult the profiler while the vite server is off.

solution

  • we could store in the collector all the vite server info for each query. but this requires us to make a query to the vite server each time: a lot of slowdown and this would considerably increase the data stored in the collector.

what do you think? would you have another idea in mind?

@andyexeter
Copy link
Collaborator

@lhapaipai I see what you mean now. Let's leave it as is. LGTM 👍

@lhapaipai lhapaipai merged commit 98cb06b into main Aug 20, 2024
9 checks passed
@lhapaipai lhapaipai deleted the feature/profiler branch August 20, 2024 12:30
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.

4 participants