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

[make:*] interactivly install composer dependencies #1466

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jrushlow
Copy link
Collaborator

@jrushlow jrushlow commented Feb 28, 2024

  • Instead of throwing an exception if $isLiveComponent but ux-live-component isn't installed, let's ask if they want us to run composer for them...

On Success (keep it clean with no output from the process):

image

On Failure (we dump composer output):

image

fixes: #1277

@jrushlow jrushlow added Feature New Feature Status: Needs Work Additional work is needed labels Feb 28, 2024
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputInterface;

/**
* Interface that all maker commands must implement.
*
* @method static string getCommandDescription()
* @method void configureComposerDependencies(DependencyManager $dependencyManager)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not a fan of touching the interface... better ideas are welcome..

*
* @internal
*/
abstract class AbstractClassDependency
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Originally went with a single class that had bool $isRequired to determine if the dependency was required or optional. I went with an abstract base model and a RequiredClassDependency && OptionalClassDependency for better DX / code review readability... Atleast that was the intent..

{
// @TODO - do we deprecate this method in favor of the one above. then remove in 2.x
// @TODO - still have plenty of work todo to determine if thats possible or a good idea...
}
Copy link
Collaborator Author

@jrushlow jrushlow Feb 28, 2024

Choose a reason for hiding this comment

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

Not a fan of touching the abstraction - but it keeps BC in check w/ the interface change. (see note on that below)

Comment on lines +83 to +84
// @TODO - with the dependencyManager in `Command` -> we can't use it outside of configure dependencies.....
$this->dependencyManager->installOptionalDependencies();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure how to go about using the manager in other maker methods (aside from configureDepends...) Gonna play around with this a bit more. but other ideas are welcome.. Thought about some sort of setter in AbstractMaker or a constructor param in the individual makers that needs this...

@@ -47,20 +51,44 @@ public function configureCommand(Command $command, InputConfiguration $inputConf
;
}

public function configureDependencies(DependencyBuilder $dependencies): void
public function configureComposerDependencies(DependencyManager $dependencyManager): void
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we can simplify. A few things:

  • Could we keep DependencyBuilder? It already records the required dependencies, which allows us to know if we need to throw an error. Now we're just going to use this to run composer require instead of throwing an error.
  • In DependencyBuilder, we have this idea of an "optional dependency". I'm wondering if we can scrap that. It seems rarely used and the usages look questionable. For example, in MakeForm, we mark validator as optional. We should be more opinionated: validation is required. We also mark orm as optional in MakeForm. That probably should be optional. But why record that as an optional dependency at all? I'd just not mention it or handle it at all.

The tl;dr is

A) Can we keep using DependencyBuilder but not worry about this optional dependencies idea anymore?
B) For packages that we discover that we need only during the interactive phase (e.g. ux-live-component after answering the "live" question), could we just make some new service available (that maker commands could DI into their constructor) that allows you to trigger the install? e.g.

if ($input->getOption('live')) {
    $this->dependencyDownloader->installPackage('symfony/ux-live-component', $io);
}

This could interactively ask them if they want to install and do it. Else throw an exception to exit the command. Or maybe we go further and just composer require packages that are needed and just notify the user as we're doing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Status: Needs Work Additional work is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make:user complains missing symfony/security package, but package is abandoned
2 participants