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

[Live] override data collector & debug command #1722

Closed
wants to merge 1 commit into from

Conversation

kbond
Copy link
Member

@kbond kbond commented Apr 11, 2024

Q A
Bug fix? no
New feature? no
Issues n/a
License MIT

When live component is installed, swap the debug command and data collector to a live component version.

This refactor enables us to add additional live-component specific details to both the debug command and the profiler.

Helps with #1720.

@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label Apr 11, 2024
Copy link
Collaborator

@WebMamba WebMamba left a comment

Choose a reason for hiding this comment

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

Hey @kbond I really like the idea!

@smnandre
Copy link
Collaborator

I’ll look at this sunday, but dont wait for me if you need before :)

@kbond
Copy link
Member Author

kbond commented Apr 12, 2024

Upon reflection, I'm thinking this PR isn't the correct direction. When adding more live component information to the debug command and profiler panel, refactoring the parent twig component classes will almost always be required. This sort of defeats the purpose of this change...

What about:

  1. Completely separate debug:live-component command
    • debug:twig-component would still show live components but we could add a "for more information on your live components, run debug:live-component..." message
  2. Completely separate live component profiler panel
    • Just like above, the twig component panel would show live components but direct users to see more detail in the live component panel

There will likely be some duplication but perhaps we can mitigate with helper classes or, in the case of the console, maybe the descriptor system (I've never used this so not sure this is viable).

I'm curious for thoughts from others.

@smnandre
Copy link
Collaborator

In general, concerning Twig & Live Component, i'm 100% for duplication over this "extend" thing we have today. Let met give my reasons before you bite me :)

  • for me TwigComponent should be moved into the TwigBundle or TwigBridge (one day, i'm not saying there is any kind of urgency there)
  • for performance reasons, i'd like the event dispatchers to be more separated
  • the Attribute extending another make us really dependent, and that -for the moment- forbid us to have dedicated features for TwigComponent (not ported to LiveComponent i mean)... but some of them could make sense.
  • the "import from bundle" rules clearly have different implication, as TwigComponent "do nothing" until called, wereas LiveComponent expose code to the external world.

So i'm all for your points. We just need to think about the "how" :)

@kbond
Copy link
Member Author

kbond commented Apr 12, 2024

Your points all make sense to me and solidify my idea to split the commands/panels. @WebMamba, wdyt?

@WebMamba
Copy link
Collaborator

When adding more live component information to the debug command and profiler panel, refactoring the parent twig component classes will almost always be required.

Do you have examples in mind?

I am not sure about that since LiveComponent is TwigComponent, I don't see why we should expose different information. More information but not different.
But tell me if you have concrete examples.

@kbond
Copy link
Member Author

kbond commented Apr 14, 2024

#1720 is the example that prompted this PR. I can imagine having details on actions and query parameters in the future.

Admittedly, I'm on the fence on what to do here. Today, I feel more inclined to proceed with this PR as is 😆

@smnandre
Copy link
Collaborator

There is nothing preventing us to change later i guess, so i'll support any choice you guys make :)

@WebMamba
Copy link
Collaborator

As long as we don't have a good reason, let's leave it as it is 😁

@kbond
Copy link
Member Author

kbond commented Apr 14, 2024

Ok, I'll finalize and merge this PR this week so we can unblock #1720.

@kbond kbond force-pushed the live-debug-profiler branch 4 times, most recently from 124df11 to 2884d50 Compare April 20, 2024 17:10
@kbond kbond requested review from WebMamba and smnandre April 20, 2024 17:12
@kbond
Copy link
Member Author

kbond commented Apr 20, 2024

I think this is ready.

Copy link
Collaborator

@WebMamba WebMamba left a comment

Choose a reason for hiding this comment

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

Currently if you run bin/console debug:live-component you also got simple TwigComponent and anonymous component. Should we filter the list to only display Live component ? If we do that should add filters to the debug:twig-component command (bin/console debug:twig-componet --anonymous, bin/console debug:twig-componet --twig-component, bin/console debug:twig-componet --live)?

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Apr 20, 2024
@kbond
Copy link
Member Author

kbond commented Apr 22, 2024

Currently if you run bin/console debug:live-component you also got simple TwigComponent and anonymous component. Should we filter the list to only display Live component ? If we do that should add filters to the debug:twig-component command (bin/console debug:twig-componet --anonymous, bin/console debug:twig-componet --twig-component, bin/console debug:twig-componet --live)?

Hah, now I'm back on the fence on what to do here. As I mentioned in #1720, it's going to be pretty cumbersome to add live-specific features to the command/profiler panel. I'm back wondering if there should be two completely separate commands and profiler panels...

@smnandre
Copy link
Collaborator

I'm back wondering if there should be two completely separate commands and profiler panels...

Still my opinion there

@smnandre
Copy link
Collaborator

I'm now persuaded having distinct commands is the better solution.

LiveComponent does not deal with anonymous templates, TwigComponent do not deal with queryurl, events, etc...

How can i help you on this ?

Copy link
Collaborator

@WebMamba WebMamba left a comment

Choose a reason for hiding this comment

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

Wait few things before merging

  • the debug:live-component does not appear on the command list when you do a bin/console since debug:live-component is just an alias
  • the debug:live-component should filter and should show only live component
  • the panel live component is name twig component and have the same icon
  • the panel live component should only display live component

WDYT?

@carsonbot carsonbot added Status: Needs Work Additional work is needed and removed Status: Reviewed Has been reviewed by a maintainer labels Apr 25, 2024
@kbond
Copy link
Member Author

kbond commented Apr 30, 2024

Closing in favour of #1812

@kbond kbond closed this Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Review Needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants