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

Speeds up loading commands #5608

Closed

Conversation

waldekmastykarz
Copy link
Member

This PR proposes 2 changes:

  • speed up loading commands by shipping a static list of commands and their file paths
  • add timings to debug mode

Rationale

Shipping a static list of commands along with the CLI makes perfect sense. After all, the list of commands for each CLI release is fixed and it doesn't make sense for us to resolve it dynamically on each command execution. By using a static list, we can significantly decrease the time of listing all commands (-3.2s) and loading a command from args (-0.2s).
The list is built by calling the build npm script and included in the .tar.gz.

Timings allow us to understand where the CLI is spending time on execution. If we notice that a particular command is slow, it'll help us understand whether it's caused by (temporary) slow API calls or the command's code that maybe inefficient. We'd collect timings on each execution (there's little overhead to it and allows us to simplify our code base) but only display them in debug mode.

@pnp/cli-for-microsoft-365-maintainers I'm looking forward to hearing what you think. If you're ok with the change, I'll finish the PR by taking care of tests and coverage.

src/cli/Cli.ts Show resolved Hide resolved
src/cli/Cli.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
scripts/write-all-commands.js Show resolved Hide resolved
@waldekmastykarz waldekmastykarz marked this pull request as ready for review November 5, 2023 16:04
@martinlingstuyl martinlingstuyl self-assigned this Nov 9, 2023
Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Nice, it really makes a very noticable difference! 🚀 Let's ship it!

@martinlingstuyl
Copy link
Contributor

Merged manually, fan-tas-tic work, you rock 🚀

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.

None yet

2 participants