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

✨ Add @percy/cli update notice #678

Merged
merged 1 commit into from
Dec 16, 2021
Merged

✨ Add @percy/cli update notice #678

merged 1 commit into from
Dec 16, 2021

Conversation

wwilsman
Copy link
Contributor

@wwilsman wwilsman commented Dec 16, 2021

What is this?

This PR adds an update notice when it detects a newer version of the CLI is available.

Before parsing command-line arguments, releases are requested from GitHub's API and compared against the currently installed CLI's version.

Releases are cached for 3 days before being re-requested, with fallbacks in place so the check still happens if there are errors when reading or writing to the cache file.

The mockfs test helper was updated to support a better spyOn syntax for memfs.fs methods and also automatically mock the current working directory.

@wwilsman wwilsman added the ✨ enhancement New feature or request label Dec 16, 2021
@wwilsman wwilsman enabled auto-merge (squash) December 16, 2021 23:29
Copy link
Contributor

@Robdel12 Robdel12 left a comment

Choose a reason for hiding this comment

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

🏁 A big key to allow global installs!

});
import('@percy/cli').then(
async ({ checkForUpdate, percy }) => {
await checkForUpdate();
Copy link
Contributor

Choose a reason for hiding this comment

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

Such a nice implementation

import pkg from '../package.json';

// filepath where the cache will be read and written to
const CACHE_FILE = path.join(__dirname, '..', '.releases');
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to not make this a json file? Would avoid having to stringify and parse it. I was annoyed doing that in the UI & localStorage

Copy link
Contributor Author

@wwilsman wwilsman Dec 17, 2021

Choose a reason for hiding this comment

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

That would not avoid having to stringify, since writeFile still needs a string or buffer.

As for reading and parsing, If you're implying to lean on require, that will not work in native ES modules (once we're there). The Node documentation recommends using fs like we're doing here, or we could add extra code to enable traditional require functionality.

import { createRequire } from 'module';
const require = createRequire(import.meta.url);
// ...
let { releases, createdAt } = require('./releases.json');

Since it's recommended to explicitly read and parse JSON files anyway, I don't see a reason to add .json to the file name in this case.

@wwilsman wwilsman merged commit 7dff315 into master Dec 16, 2021
@wwilsman wwilsman deleted the ww/cli-update-notice branch December 16, 2021 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants