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

fix: update @heroku/cli-command #62

Merged
merged 1 commit into from
Jan 6, 2025
Merged

Conversation

k80bowman
Copy link
Contributor

Fixes a bug that caused some requests to time out and print an error message with a type error.

Testing

  • Pull down this branch and run yarn && yarn build
  • Run ./bin/run ai:models:call inference-animate-42224 -a test-cli-plugin-ai -p "what is candy?" (internal team members should have access to this app and model)
  • Request should succeed

@k80bowman k80bowman requested a review from a team as a code owner January 6, 2025 15:35
Copy link
Contributor

@sbosio sbosio left a comment

Choose a reason for hiding this comment

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

I tested and all works as expected, so I'm approving. But I still see references in yarn.lock to the old http-call package.

They come from two sources:

  • One is oclif which is set to a really old version 2.2.0. I believe this could be updated to a newer version of oclif and perhaps it doesn't longer depend on http-call? Because this is a dev tool only used at build / packaging, I guess.
  • The other one is the plugin that warns about a new version availability, but it's also a transitive dependency for oclif, maybe it gets removed with just updating oclif to a newer version.

@k80bowman
Copy link
Contributor Author

Ooh good call. We had it set to oclif v2.2.0 because bumping it to a later version required Node 18. I'll try bumping it now and see what happens, it should be fine, though.

Unfortunately that won't fix the reference from the @oclif/plugin-warn-if-update-available package, however. I can submit a PR to update that repo, but it won't be updated immediately.

@k80bowman
Copy link
Contributor Author

Actually, in the interest of publishing this fix today, I am going to hold off on bumping the oclif version and do that in a separate PR. That way I can get @oclif/plugin-warn-if-update-available updated as well.

@k80bowman k80bowman merged commit 1ac8c1b into main Jan 6, 2025
6 checks passed
@k80bowman k80bowman deleted the k80/update-cli-command branch January 6, 2025 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants