Add `--cache-location` option #13019

sosukesuzuki · 2022-06-18T14:26:08Z

Description

Fixes #13010

Checklist

I’ve added tests to confirm my change works.
(If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).
(If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
I’ve read the contributing guidelines.

TODO

Create issue: Add --cache-location option #13019 (comment)
Create issue: JSON checks with low memory usage Add --cache-location option #13019 (comment)
Create issue: More safety JSON checks Add --cache-location option #13019 (comment)

✨Try the playground for this PR✨

fisker · 2022-06-19T02:24:56Z

If multiple projects use the same cache dir, we'll hit the wrong cache, I guess use absolute path make sense now.

HolgerJeromin · 2022-06-20T13:29:10Z

src/cli/find-cache-file.js

+/**
+ * If a file path is passed, that file is used as the cache file.
+ * If a directory path is passed,
+ *    a file with the name hashed process.cwd() is created in that directory and used as a cache file.


Thanks for your work!

I think that information should be included in the changelog and CLI doc.

docs/cli.md

sosukesuzuki · 2022-06-22T12:35:30Z

If multiple projects use the same cache dir, we'll hit the wrong cache, I guess #12800 (comment) make sense now.

@fisker Sorry I don't know what do you mean. Could you share some examples?

sosukesuzuki · 2022-06-25T17:06:40Z

@fisker Friendly ping

fisker · 2022-06-27T03:29:53Z

As far as I understand, we store path related to PWD in cache, if multiple projects use a global dir to cache , and they have files with same name, won't it cause problem or at least make another project loose cache?

alexander-akait · 2022-06-27T11:17:13Z

@fisker I think it should be exptected, otherwise cache on CI will not work

fisker · 2022-06-28T03:03:47Z

IDK, I won't use it anyway.

fisker · 2022-06-28T03:06:06Z

I have one more concern, is it a good idea to accept both directory and file path?

sosukesuzuki · 2022-07-09T04:32:44Z

I have one more concern, is it a good idea to accept both directory and file path?

No, I'll fix it.

sosukesuzuki · 2022-07-09T06:44:10Z

done

sosukesuzuki · 2022-07-09T06:48:18Z

If multiple projects use the same cache dir, we'll hit the wrong cache, I guess #12800 (comment) make sense now.

@fisker Are you saying that we should only accept absolute paths as --cache-location?

fisker · 2022-07-09T14:06:59Z

No, I meant we should use absolute path of formatting file when save cache.

sosukesuzuki · 2022-07-10T04:41:22Z

No, I meant we should use absolute path of formatting file when save cache.

@fisker I got it. So can we merge this PR for now? I don't want to increase the size of one PR. I'll create new PR to use absolute path later.

fisker · 2022-07-10T10:26:27Z

Did you mean to support path to cache file? The changelog and docs still says "directory or file path".

fisker · 2022-07-10T10:32:53Z

src/cli/format.js

+      const stat = await statSafe(cacheFilePath);
+      if (stat) {
+        await fs.unlink(cacheFilePath);
+      }


If I didn't get this wrong, this line is going to remove default cache file when running Prettier without --cache, but I don't think we should. People may use Prettier without --cache temporarily.

This is a direct implementation of ESLint's behavior and has nothing to do with --cache-location. It will be fixed in another PR.

fisker · 2022-07-10T10:37:40Z

src/cli/find-cache-file.js

+  }
+  const cwd = process.cwd();
+  const normalized = path.normalize(cacheLocation);
+  return path.join(cwd, normalized);


I don't think it's a good idea to use the input path directly, people may pass wrong path by accident, if path exists, we should verify

It's a file not a directory

Verify it's a valid Prettier cache file (maybe add a mark to the cache when save cache)

It's a file not a directory

done! 9789e04

Verify it's a valid Prettier cache file (maybe add a mark to the cache when save cache)

How?

How?

I'm not sure how the cache file is created, but is it a JSON file? There should be a way to detect if the file is created by Prettier.

That file is a JSON file, but it is almost always huge. There is an overhead in making sure it is in the correct format. I don't think its validation is worth the overhead.

Personally, I hope it's safer, I'll be pissed if prettier --cache --cache-location index.js src(forgot cache file path) saves cache to my index.js

afcbd3c

I've added JSON validation. But it consumes a lot of memory. We should use Stream in the future to implement JSON checks with low memory usage.

Is there a specially key inside that we can confirm it's a Prettier cache file?

I don't know, need to read the file-entry-cache implementation

I want to break up the PR because one big PR is exhausting. This is listed in the TODO of this PR.

fisker · 2022-07-10T15:55:05Z

src/cli/constant.js

@@ -76,6 +76,10 @@ const options = {
    description: "Only format changed files. Cannot use with --stdin-filepath.",
    type: "boolean",
  },
+  "cache-location": {
+    description: "Path to the cache file or directory",


Suggested change

description: "Path to the cache file or directory",

description: "Path to the cache file",

fisker · 2022-07-10T18:05:27Z

src/cli/find-cache-file.js

+async function findCacheFileFromOption(cacheLocation) {
+  const cwd = process.cwd();
+  const normalized = path.normalize(cacheLocation);
+  const cacheFile = path.join(cwd, normalized);


Should we run path.isAbsolute() check first? Not sure how it works for /foo and D:\foo(Windows, different partition))

process.cwd() returns absolute path. So I don't think we should run path.isAbsolute check.

I didn't mean check cwd, cacheLocation.

Maybe path.resolve(cacheLocation) is enough.

Co-authored-by: Holger Jeromin <mailgithub@katur.de>

* Draft * Update * Generate * Add `truncate` * Address review * Regen * Update * Reword changelog for #13016 * Edit intro * Edit changelog for #13019 * Regenerate blog post * Address review Co-authored-by: Georgii Dolzhykov <thorn.mailbox@gmail.com>

* Implement searching cache file from `--cache-location` option * Add tests * Add docs * Fix * Fix doc * Add changelog * Update snapshots * Update docs * Update docs/cli.md Co-authored-by: Holger Jeromin <mailgithub@katur.de> * Fix changelog * Remove directory support * Update docs * Update tests * Update docs * Add validation for dir * Fix comment * Update option description * Validate JSON * Use `path.resolve` Co-authored-by: Holger Jeromin <mailgithub@katur.de>

* Draft * Update * Generate * Add `truncate` * Address review * Regen * Update * Reword changelog for prettier#13016 * Edit intro * Edit changelog for prettier#13019 * Regenerate blog post * Address review Co-authored-by: Georgii Dolzhykov <thorn.mailbox@gmail.com>

HolgerJeromin reviewed Jun 20, 2022

View reviewed changes

HolgerJeromin reviewed Jun 22, 2022

View reviewed changes

docs/cli.md Outdated Show resolved Hide resolved

sosukesuzuki force-pushed the cache-location branch from 24f06af to 45f441b Compare July 9, 2022 06:24

sosukesuzuki mentioned this pull request Jul 9, 2022

node_modules/.cache/prettier #13032

Open

sosukesuzuki requested a review from fisker July 10, 2022 04:41

fisker reviewed Jul 10, 2022

View reviewed changes

sosukesuzuki force-pushed the cache-location branch from 8a6c246 to 75a8bdb Compare July 10, 2022 14:03

fisker reviewed Jul 10, 2022

View reviewed changes

sosukesuzuki added 7 commits July 16, 2022 17:25

Implement searching cache file from --cache-location option

e820934

Add tests

6c892a1

Add docs

92eda3a

Fix

f212950

Fix doc

e91420f

Add changelog

ec9eda7

Update snapshots

ce2704f

sosukesuzuki and others added 11 commits July 16, 2022 17:25

Update docs

c0e7dc6

Update docs/cli.md

4b252dd

Co-authored-by: Holger Jeromin <mailgithub@katur.de>

Fix changelog

f5142ca

Remove directory support

a541b2e

Update docs

a0e611c

Update tests

ca0ed76

Update docs

f83a70a

Add validation for dir

e3bd652

Fix comment

6e680da

Update option description

2e228d9

Validate JSON

afcbd3c

sosukesuzuki force-pushed the cache-location branch from 24c9294 to afcbd3c Compare July 16, 2022 11:47

Use path.resolve

c5b2063

sosukesuzuki requested a review from fisker July 31, 2022 13:49

fisker approved these changes Jul 31, 2022

View reviewed changes

sosukesuzuki merged commit 955553b into prettier:main Jul 31, 2022

sosukesuzuki deleted the cache-location branch July 31, 2022 13:58

kachkaev added this to the 2.8 milestone Nov 16, 2022

thorn0 added a commit to sosukesuzuki/prettier that referenced this pull request Nov 22, 2022

Edit changelog for prettier#13019

c83d850

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `--cache-location` option #13019

Add `--cache-location` option #13019

sosukesuzuki commented Jun 18, 2022 •

edited

fisker commented Jun 19, 2022 •

edited

HolgerJeromin Jun 20, 2022 •

edited

sosukesuzuki commented Jun 22, 2022

sosukesuzuki commented Jun 25, 2022

fisker commented Jun 27, 2022

alexander-akait commented Jun 27, 2022

fisker commented Jun 28, 2022

fisker commented Jun 28, 2022

sosukesuzuki commented Jul 9, 2022

sosukesuzuki commented Jul 9, 2022

sosukesuzuki commented Jul 9, 2022

fisker commented Jul 9, 2022 •

edited

sosukesuzuki commented Jul 10, 2022 •

edited

fisker commented Jul 10, 2022

fisker Jul 10, 2022 •

edited

sosukesuzuki Jul 10, 2022

fisker Jul 10, 2022

sosukesuzuki Jul 10, 2022

fisker Jul 10, 2022 •

edited

sosukesuzuki Jul 10, 2022

fisker Jul 10, 2022

sosukesuzuki Jul 16, 2022

fisker Jul 16, 2022

sosukesuzuki Jul 16, 2022

sosukesuzuki Jul 16, 2022

sosukesuzuki Jul 31, 2022

fisker Jul 10, 2022

sosukesuzuki Jul 10, 2022

fisker Jul 10, 2022

sosukesuzuki Jul 16, 2022

fisker Jul 16, 2022

fisker Jul 16, 2022

sosukesuzuki Jul 16, 2022

	description: "Path to the cache file or directory",
	description: "Path to the cache file",

Add --cache-location option #13019

Add --cache-location option #13019

Conversation

sosukesuzuki commented Jun 18, 2022 • edited

Description

Checklist

TODO

fisker commented Jun 19, 2022 • edited

HolgerJeromin Jun 20, 2022 • edited

Choose a reason for hiding this comment

sosukesuzuki commented Jun 22, 2022

sosukesuzuki commented Jun 25, 2022

fisker commented Jun 27, 2022

alexander-akait commented Jun 27, 2022

fisker commented Jun 28, 2022

fisker commented Jun 28, 2022

sosukesuzuki commented Jul 9, 2022

sosukesuzuki commented Jul 9, 2022

sosukesuzuki commented Jul 9, 2022

fisker commented Jul 9, 2022 • edited

sosukesuzuki commented Jul 10, 2022 • edited

fisker commented Jul 10, 2022

fisker Jul 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisker Jul 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `--cache-location` option #13019

Add `--cache-location` option #13019

sosukesuzuki commented Jun 18, 2022 •

edited

fisker commented Jun 19, 2022 •

edited

HolgerJeromin Jun 20, 2022 •

edited

fisker commented Jul 9, 2022 •

edited

sosukesuzuki commented Jul 10, 2022 •

edited

fisker Jul 10, 2022 •

edited

fisker Jul 10, 2022 •

edited