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

feat: load command's actions async to improve performance #6180

Merged
merged 70 commits into from
Nov 22, 2023

Conversation

sarahetter
Copy link
Contributor

🎉 Thanks for submitting a pull request! 🎉

Summary

Thanks to @rjbeers investigation, we've found that loading in individual commands' action functions asynchronously leads to faster load time, as we aren't loading in every single package on each command run. I've done this for each command we have in the CLI.

I've also added Typescript types for commands and their actions in this PR.

Before After
netlify -v - around 2 seconds netlify -v on main netlify -v - just over 0.5 secondsnetlify -v on branch
netlify status - around 4 seconds before netlify status - around 2.75 seconds after

For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)
mothman-alien

})

program
.command('addons:delete', { hidden: true })
Copy link
Member

Choose a reason for hiding this comment

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

The indentation looks a bit off here. I don't think we've changed the Prettier settings to run on .mts files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah @ericapisani is this in the plans with what you're doing now?

Copy link
Member

Choose a reason for hiding this comment

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

I believe we need to add the new extensions here:

"prettier": "--ignore-path .eslintignore --loglevel=warn \"{src,tools,scripts,tests,.github}/**/*.{mjs,cjs,js,md,yml,json,html}\" \"*.{mjs,cjs,js,yml,json,html}\" \".*.{mjs,cjs,js,yml,json,html}\" \"!CHANGELOG.md\" \"!**/*/package-lock.json\" \"!.github/**/*.md\""
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added config in #6187, and ran format. Should be prettier now :D

eduardoboucas
eduardoboucas previously approved these changes Nov 17, 2023
Copy link
Member

@eduardoboucas eduardoboucas left a comment

Choose a reason for hiding this comment

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

🚀

@sarahetter sarahetter added the automerge Add to Kodiak auto merge queue label Nov 17, 2023
@kodiakhq kodiakhq bot removed the automerge Add to Kodiak auto merge queue label Nov 20, 2023
Copy link
Contributor

kodiakhq bot commented Nov 20, 2023

This PR currently has a merge conflict. Please resolve this and then re-add the automerge label.

@sarahetter sarahetter added the automerge Add to Kodiak auto merge queue label Nov 22, 2023
@kodiakhq kodiakhq bot merged commit 5c61169 into main Nov 22, 2023
35 checks passed
@kodiakhq kodiakhq bot deleted the sarahetter/ct-276-improve-performance branch November 22, 2023 21:51
@pieh
Copy link
Contributor

pieh commented Nov 24, 2023

@sarahetter Is it possible that this change could cause some issues with lambda execution timeouts? Tests in https://github.com/netlify/netlify-plugin-gatsby started failing because of errors like:

Function __api has returned an error: Task timed out after 10.00 seconds
    at new TimeoutError (/opt/hostedtoolcache/node/20.9.0/x64/lib/node_modules/netlify-cli/node_modules/lambda-local/build/lib/utils.js:119:28)
    at Context.<anonymous> (/opt/hostedtoolcache/node/20.9.0/x64/lib/node_modules/netlify-cli/node_modules/lambda-local/build/lib/context.js:110:19)
    at listOnTimeout (node:internal/timers:573:17)
    at processTimers (node:internal/timers:514:7)

Downgrading CLI to 17.6.0 "fixes" those timeouts.

Using 17.7.0 test run example - https://github.com/netlify/netlify-plugin-gatsby/actions/runs/6983007552/job/19003322129?pr=722 (above snippet copied from it) - it seems to happen every time now with that version of CLI
Using 17.6.0 test run example is fine https://github.com/netlify/netlify-plugin-gatsby/actions/runs/6982743678/job/19002516599?pr=721

I don't quite know if it's possible this would be the case as this is pretty hefty change in this PR - but would async loading part possibly be part of lambda execution time now, meaning that part of 10s lambda execution timeout would be taken by async loading?

Other change in 17.7.0 seems to be just applying prettier and file renames so it seems less likely to me that it could cause it? Also I couldn't really reproduce the timeout issue locally, so hard for me to debug it to provide more information

@sarahetter
Copy link
Contributor Author

@pieh I just took a look through the netlify-plugin-gatsby tests and it looks like they're using netlify dev - which didn't actually get the async treatment in this PR because I had failing tests, so it was kept as is. So for now, I'm just not sure.

@eduardoboucas I think I'll need your help trying to debug this. I'm not familiar with the dev/functions handling code within the CLI, and can't guess as to what's causing these timeouts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Add to Kodiak auto merge queue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants