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

Implement lazy-loading of cli commands #419

Merged
merged 4 commits into from
Jun 10, 2021

Conversation

vladgorenkin
Copy link
Contributor

@vladgorenkin vladgorenkin commented Jun 8, 2021

Q A
Bugfix?
Breaks BC? ✔️
New feature? ✔️

This PR implements lazy-loading of CLI commands to prevent unneeded commands from being constructed. This speeds up CLI initialization and solves situations when one command's dependency breaks all other commands (e.g. a command whose dependency uses result of ORM query will break migrate:init when no migrations were applied).

Lazy-loaded command declaration is done through upgrading Command::NAME's visibility to public so locators can extract command's name (and optionaly description) from command without constructing it.

Example:

class LazyCommand extends Spiral\Console\Command {
    public const NAME = 'lazy';

    public function __construct() {
        sleep(5); // this will only be run when command is executed, not on console initialization
    }
}

@codecov
Copy link

codecov bot commented Jun 8, 2021

Codecov Report

Merging #419 (8aab1a6) into master (bf8f9bf) will increase coverage by 0.02%.
The diff coverage is 97.77%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #419      +/-   ##
============================================
+ Coverage     84.35%   84.38%   +0.02%     
- Complexity     8722     8737      +15     
============================================
  Files          1188     1192       +4     
  Lines         29671    29726      +55     
============================================
+ Hits          25029    25083      +54     
- Misses         4642     4643       +1     
Impacted Files Coverage Δ
src/Framework/Console/CommandLocator.php 83.33% <66.66%> (-6.67%) ⬇️
src/Console/src/StaticLocator.php 100.00% <100.00%> (ø)
src/Console/src/Traits/LazyTrait.php 100.00% <100.00%> (ø)
src/Console/tests/Fixtures/LazyLoadedCommand.php 100.00% <100.00%> (ø)
src/Console/tests/LazyTest.php 100.00% <100.00%> (ø)
src/AnnotatedRoutes/src/RouteLocator.php 100.00% <0.00%> (ø)
...atedRoutes/tests/App/Controller/PageController.php 100.00% <0.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf8f9bf...8aab1a6. Read the comment docs.

@wolfy-j wolfy-j requested a review from SerafimArts June 8, 2021 10:58
@SerafimArts
Copy link
Contributor

SerafimArts commented Jun 8, 2021

@vladgorenkin It breaks BC. Therefore, we can only accept at v2.9

In addition, I don't really want to be tied to agreements with "magic constants". However, I understand that this style has already been adopted in Spiral 2.x and should be followed.

@vladgorenkin
Copy link
Contributor Author

It breaks BC

In what way? Dependency on LazyCommand? No problem in waiting till 2.9 though, no hurry here.

In addition, I don't really want to be tied to agreements with "magic constants"

I absolutely agree, I do not like magic solutions either although it seemed to me as the most suitable taking into account existing logic. This solution was also partly inspired by Symfony's way of declaring lazy commands (by declaring $defaultName property static).

The problem is we need to extract name and description of a command to register it in Application so NAME and DESCRIPTION need to be public. If you have any better solution in mind please let me know.

@SerafimArts
Copy link
Contributor

In what way? Dependency on LazyCommand?

Yep. This requires bringing up a version of the Symfony's components. There is no such thing now: https://github.com/spiral/framework/pull/419/checks?check_run_id=2772546255

@SerafimArts
Copy link
Contributor

@vladgorenkin Can you raise a version of the symfony dependencies to pass all the tests?

@SerafimArts SerafimArts added this to the 2.9 milestone Jun 10, 2021
@vladgorenkin
Copy link
Contributor Author

✔️

@SerafimArts
Copy link
Contributor

This is not quite the correct way. When the build starts, everything will break.

The final composer file is built from all existing packages using vendor/bin/monorepo-builder merge command. You should fix the versions for all packages (inside src dir) and run this command.

Thanks anyway. This is not critical, I will fix everything when I do (merge) this PR.

@vladgorenkin
Copy link
Contributor Author

vladgorenkin commented Jun 10, 2021

Just for the sake of my own education tried to resolve conflicts and generate composer.json per your instructions. Would you have a look and tell me if everyhing is ok?

@SerafimArts
Copy link
Contributor

Yes. 👍

I see that there are problems with jobs and migrations. This seems to be an automation problem, since these packages are optional and have not yet been versioned along with the framework. Thank you for noticing this and correcting it too.

@SerafimArts SerafimArts merged commit 992e53f into spiral:master Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants