Retrieve latest CLI version in the background#2189
Conversation
amcaplan
requested review from
a team,
hannachen and
molly-yu
and removed request for
a team
amcaplan
force-pushed
the
check-for-new-version-in-background
branch
from
28c7079 to
301527a
Compare
Currently we check for a new version as a blocking process. This isn't a good experience for CLI users, for whom the command hangs once daily until we check for a new version. This approach will fork the process and check in the background, saving the result and warning the user the next time they run a command. Note this isn't ideal for CLI environments which might run 1 command per CLI run and never show the warning. We might choose to mitigate by checking inline on non-TTY terminals.
amcaplan
force-pushed
the
check-for-new-version-in-background
branch
from
ab5140c to
6c9df66
Compare
| def with_stubbed_context(&block) | ||
| @old_config = ShopifyCLI::Config | ||
| without_warnings do | ||
| ShopifyCLI.const_set(:Config, Class.new do |
There was a problem hiding this comment.
const_set here and below displays a warning, since the constant has already been set. So I disable warnings here to avoid unhelpful output in tests.
| rescue | ||
| nil | ||
| ensure | ||
| ShopifyCLI::Config.set(VERSION_CHECK_SECTION, LAST_CHECKED_AT_FIELD, Time.now.to_i) |
There was a problem hiding this comment.
Still wait until tomorrow to try if something went wrong
There was a problem hiding this comment.
Something is wrong. This doesn't work as intended. Version check repeats on every command. See #2382
gonzaloriestra
left a comment
gonzaloriestra
left a comment
There was a problem hiding this comment.
The code LGTM and it works as expected! 👏
|
Fork isn't supported on Windows 11, have been trying to figure out what was wrong. See my stacktrace after running |
|
@AnwarElbo oh, we didn't realize about that, thanks for the notice. I'll prepare a bugfix as soon as possible. |
|
@AnwarElbo it should be fixed in v2.16.1, sorry for the inconvenience! |
| end | ||
| end | ||
| latest_version = ShopifyCLI::Config.get(VERSION_CHECK_SECTION, LATEST_VERSION_FIELD, default: ShopifyCLI::VERSION) | ||
| latest_version unless latest_version == ShopifyCLI::VERSION |
There was a problem hiding this comment.
This will always return the latest_version even when (time_of_last_check + VERSION_CHECK_INTERVAL) < (now = Time.now.to_i) is false.
Fixed in PR #2453
4 participants
WHY are these changes introduced?
Inspired by #2052
Currently we check for a new version as a blocking process. This isn't a good experience for CLI users, for whom the command hangs once daily until we check for a new version.
WHAT is this pull request doing?
This approach will fork the process and check in the background, saving the result and warning the user the next time they run a command.
Note this isn't ideal for CLI environments which might run 1 command per CLI run and never show the warning. We might choose to mitigate by checking inline on non-TTY terminals.
How to test your changes?
if/elsifor just copy the lines to after the conditional)bin/shopify versiona few times and you'll start to see the new version warning (hopefully on round 2)Update checklist