-
Notifications
You must be signed in to change notification settings - Fork 367
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
Conversation
src/commands/addons/addons.mts
Outdated
}) | ||
|
||
program | ||
.command('addons:delete', { hidden: true }) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
Line 79 in 7ab484b
"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\"" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
This PR currently has a merge conflict. Please resolve this and then re-add the |
@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:
Downgrading CLI to 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 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 |
@pieh I just took a look through the @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 |
🎉 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.
For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)