Add `--cache-location` option by sosukesuzuki · Pull Request #13019 · prettier/prettier

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

+/**
+ * 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.

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

+      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

+  }
+  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

    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

+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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]> * 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 <[email protected]>

* 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 <[email protected]>

HolgerJeromin reviewed Jun 20, 2022

View reviewed changes

HolgerJeromin reviewed Jun 22, 2022

View reviewed changes

Comment thread docs/cli.md Outdated

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

Closed

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 <[email protected]>

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

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

Uh oh!

Conversation

sosukesuzuki commented Jun 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

TODO

Uh oh!

fisker commented Jun 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HolgerJeromin Jun 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sosukesuzuki commented Jun 22, 2022

Uh oh!

sosukesuzuki commented Jun 25, 2022

Uh oh!

fisker commented Jun 27, 2022

Uh oh!

alexander-akait commented Jun 27, 2022

Uh oh!

fisker commented Jun 28, 2022

Uh oh!

fisker commented Jun 28, 2022

Uh oh!

sosukesuzuki commented Jul 9, 2022

Uh oh!

sosukesuzuki commented Jul 9, 2022

Uh oh!

sosukesuzuki commented Jul 9, 2022

Uh oh!

fisker commented Jul 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sosukesuzuki commented Jul 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fisker commented Jul 10, 2022

Uh oh!

fisker Jul 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisker Jul 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sosukesuzuki commented Jun 18, 2022 •

edited

Loading

fisker commented Jun 19, 2022 •

edited

Loading

HolgerJeromin Jun 20, 2022 •

edited

Loading

fisker commented Jul 9, 2022 •

edited

Loading

sosukesuzuki commented Jul 10, 2022 •

edited

Loading

fisker Jul 10, 2022 •

edited

Loading

fisker Jul 10, 2022 •

edited

Loading