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 --cache
CLI option
#12800
Add --cache
CLI option
#12800
Conversation
Great job! |
|
src/cli/constant.js
Outdated
"cache-location": { | ||
type: "path", | ||
description: "Path to the cache file or directory", | ||
default: ".prettiercache", |
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.
Default to node_modules/.cache/prettier
? https://github.com/avajs/find-cache-dir, #5910 also use this.
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.
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.
in my opinion, node_modules/.cache is a really bad place to put the cache.
Because when using yarn, if any dependency is upgraded (not just prettier), then .cache is cleared.
find-cache-dir does allow specifying a CACHE_DIR so I can use that to get around it, but if you use alot of packages and upgrade daily and use yarn, it make the cache option useless.
@fisker /cc @alexander-akait The current Prettier JavaScript API does not take a file path as an argument. Therefore, I don't think it is possible to implement a file-based cache in the API. |
src/cli/find-cache-file.js
Outdated
* Find default cache file (`./node_modules/.cache/prettier/.prettiercache`) using https://github.com/avajs/find-cache-dir | ||
*/ | ||
function findDefaultCacheFile() { | ||
const cacheDirThunk = findCacheDir({ name: "prettier", thunk: 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.
src/cli/utils.js
Outdated
* @returns {string} | ||
*/ | ||
function createHash(source) { | ||
return crypto.createHash("md5").update(source).digest("hex"); |
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.
https://github.com/prettier/prettier/pull/5910/files#r273623309, or maybe some other non-cryptographic-hash? I use sdbm
sometime.
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.
sdbm looks good f1b2c74
function getMetadataFromFileDescriptor(fileDescriptor) { | ||
return fileDescriptor.meta; | ||
} |
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.
Why don't we use fileDescriptor.meta
directly? This just making code less readable.
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.
Just for type checking...
default: false, | ||
description: "Only format changed files", | ||
type: "boolean", | ||
}, |
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.
Should we enable it by default?
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.
At least we cannot enable it by default until v3
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 am thinking about it too, but most of tools disable cache by default - not to waste extra disk space without allowing, also some CI can work in read only access or with write access only for specified dirs, so if we enable it by default in v2, we can break CI
src/cli/constant.js
Outdated
"cache-location": { | ||
description: | ||
"Path to the cache file or directory\nDefaults to node_modules/.cache/prettier/.prettiercache", | ||
type: "path", |
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.
Do we need this?
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 added cache-location
options because ESLint has same option. But, I think we can remove this.
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.
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 think it makes sense to leave cache-location
. It's a good idea to leave users the option to control where this cache is stored. It's useful in CI at least. Usually, people cache their node_modules by yarn.lock and it could lead to undesired caching of prettier.
Also removing node_modules for reinstall is still a widely spreaded approach and it will wipe all prettier cache
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.
Why don't we add it if someone request in future?
return optionsHashCache.get(options); | ||
} | ||
const hash = createHash( | ||
`${prettierVersion}_${nodeVersion}_${stringify(options)}` |
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.
We allow functions in options
, not sure how should we handle this.
plugins: [require('prettier-plugin-foo')]
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 have no idea to serialize an option that includes functions for plugins. What do you think about to mention about behavior for plugins and caching.
If you are using plugins in your Prettier config file, caching may not work well. If caching does not work well after updating the plugin, try running Prettier without the `--cache` option to clear the cache.
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.
* @param {any} options | ||
*/ | ||
existsAvailableFormatResultsCache(filePath, options) { | ||
const fileDescriptor = this.fileEntryCache.getFileDescriptor(filePath); |
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.
Feel not safe without file content, but that will be slow.
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.
For minimum speed and security, why not check the length of the file content rather than the content of the file?
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.
In this case it's not usable in CI. ESLint, for example, has --cache-strategy which allows to use metadata or content for detecting changed 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.
Thanks, I've added --cache-strategy
option!
@@ -362,17 +368,28 @@ async function formatFiles(context) { | |||
|
|||
const start = Date.now(); | |||
|
|||
const existsCache = formatResultsCache?.existsAvailableFormatResultsCache( | |||
filename, |
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.
Should we use absolute path here?
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.
Why? I think current implementation is enough.
How realistic would it be to cache markdown code blocks on their own (as if there were separate files)? I am maintaining prettier-plugin-elm which calls |
const { createHash } = require("./utils.js"); | ||
|
||
const optionsHashCache = new WeakMap(); | ||
const nodeVersion = process && process.version; |
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.
Why we need it? Something in our code depend on Node.js version?
Good question. But we have a glob, can we use it? |
I think @sosukesuzuki is right. it's impossible to add this in API, unless we add something like |
--cache
and --cache-location
--cache
CLI optio
changelog_unreleased/cli/12800.md
Outdated
@@ -0,0 +1,19 @@ | |||
#### Add `--cache` CLI option (#12800 by @sosukesuzuki) |
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.
Missed cache-strategy
in title.
changelog_unreleased/cli/12800.md
Outdated
|
||
##### `--cache` | ||
|
||
When you want to format only changed files, you can run Prettier with `--cache` flag. Enabling this flag can dramatically improve running time. |
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 sounds like CLI will skip unchanged files, but it's not.
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.
What is wrong?
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.
Even if file is unchanged, but last run was not using --cache
we'll format it.
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.
Or only config changed, but file didn't change.
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 got it. However, I have no idea to better sentence now... I'll figure it out later.
changelog_unreleased/cli/12800.md
Outdated
@@ -0,0 +1,19 @@ | |||
#### Add `--cache` CLI option (#12800 by @sosukesuzuki) | |||
|
|||
Two new CLI options have been added for a caching system similar to [ESLint's one](https://eslint.org/docs/user-guide/command-line-interface#caching). Please see the [documentation](https://prettier.io/docs/en/cli.html#--cache) for details. |
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 link to docs seems unnecessary, you already explained here.
@kachkaev Sorry I forgot to reply to you. Since the cache feature implemented here is a per-file cache, it is not possible to use this feature for caching Markdown code blocks. If caching Markdown code blocks is beneficial enough for the Prettier core, it could be implemented in the future( But don't expect...) |
Co-authored-by: Simon Lydell <simon.lydell@gmail.com>
@fisker @alexander-akait @lydell I will merge for now, but if you have any concerns, please comment again before the release. |
output = result.formatted; | ||
} catch (error) { | ||
handleError(context, filename, error, printedFilename); | ||
continue; | ||
} | ||
|
||
formatResultsCache?.setFormatResultsCache(filename, options); |
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.
If we’re running without --write
, this incorrectly caches the unformatted file rather than the formatted file, which makes the --cache
option unusable. Please merge #13016.
* Install `file-entry-cache` and types * Implement `FormatResultsCache` * Add `--cache` and `--cache-location` option * Implement `--cache` and `--cache-location` * Add tests for `--cache` * Add tests for `--cache-location` * Avoid spellcheck error * Update snapshots for new options * Add `groupBy` for website * Use `fs.promises` instead of `fs/promises` * Use `rimraf` instead of `fs.rm` * Fix properties order of `options` * Install `find-cache-dir` and types * Change default cache file location to `node_modules/.cache/prettier` * Fix tests following to change default cache file location * Update snapshots for changing default cache file location * Set `os.tmpdir()` as a fallback for default cache dir * Add docs for `--cache` and `--cache-location` * Use `sdbm` instead of `node:crypto` * Add changelog * Fix `getHashOfOptions` * Remove `cache-location` * `prettiercache` -> `prettier-cache` * Remove cache file when run Prettier without `--cache` option * Implement `--cache-strategy` * Add tests for invalid cache strategy * Fix lint problems * Tweaks tests * Add tests for timestamp * Add tests for `--cache-strategy` * Add docs for cache-strategy * Address review * Fix `findCacheFile` * Use Set.prototype.has * Throw error with --stdin-filepath * Update snapshots * Fix error for cache and stdin * Use string constructor instead of toString * Update changelog * Use flag validation * Fix `cache-strategy` definition * Update docs * Mark highlihgt * Throw error for `--cache-strategy` without `--cache` * Update docs * Fix typo * Updates snapshots * Update docs/cli.md Co-authored-by: Simon Lydell <simon.lydell@gmail.com> * Fix error message * Remove `:::` syntax * Defaults `content` * Update docs * Fix by Prettier Co-authored-by: Simon Lydell <simon.lydell@gmail.com>
Description
Fixes #5853
Two new CLI options have been added for a caching system similar to ESLint's one. Please see the documentation for details.
--cache
When you want to format only changed files, you can run Prettier with
--cache
flag. Enabling this flag can dramatically improve running time.--cache-strategy
Strategy for the cache to use for detecting changed files. Can be either
metadata
orcontent
. If no strategy is specified, metadata will be used.Checklist
docs/
directory).changelog_unreleased/*/XXXX.md
file followingchangelog_unreleased/TEMPLATE.md
.✨Try the playground for this PR✨