Fix --cache set only if with --write or no-different by Milly · Pull Request #13016 · prettier/prettier

Milly · 2022-06-16T14:20:00Z

Description

Do not save formatted cache if without --write option and the file is modifies by prettier.

Before fixed, always save formatted cache if with --cache option.
Therefore, unformatted files will never be formatted.

I think this is a bug.

Before fixed:

> prettier --cache foo.js
# Show formatted contents of `foo.js`.
# Then the cache is created and `foo.js` is flagged as already formatted.

> prettier --cache --write foo.js
foo.js 2ms (cached)
# "... (cached)" means that the file is already formatted and do nothing this time.

After fixed with this PR:

> prettier --cache foo.js
# Show formatted contents of `foo.js`.

> prettier --cache --write foo.js
foo.js 2ms
# `foo.js` has been formatted and overwritten.

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.

✨Try the playground for this PR✨

fisker · 2022-06-16T15:11:45Z

Why don't we update cache when file actually successfully written?

Milly · 2022-06-16T15:30:03Z

Why don't we update cache when file actually successfully written?

Doing so adds too complexity due to performanceTestFlag condition.
I made a fix to reduce the diff.

Conditions to be cached

Options	File needs to be formatted	Should be cached
`--cache`	Yes	No
`--cache`	No	Yes
`--cache --write`	Yes	Yes
`--cache --write`	No	Yes

Milly · 2022-06-16T15:34:49Z

Should we not write the cache file if performanceTestFlag is specified?
I don't understand.

fisker · 2022-06-16T15:39:50Z

We don't need care about performanceTestFlag that's for debugging.

Milly · 2022-06-16T16:29:08Z

Why don't we update cache when file actually successfully written?

I fixed it.

Milly · 2022-06-17T06:05:28Z

This fixes #13015

Milly · 2022-06-17T08:26:22Z

I found an additional problem.
Fixing now.

> prettier --cache --write foo.js
foo.js 2ms
# Cache created.

> vi foo.js
# Edit to unformatted.

> prettier --list-different --cache foo.js
foo.js
# Cache is updated because the file entry is already exist in cache and update all.

> prettier --list-different --cache foo.js
# Expected `foo.js` will be output, but nothing.

Milly · 2022-06-17T10:30:51Z

Fixed it.

Milly · 2022-06-17T10:31:03Z

refs #12800

Milly · 2022-06-20T09:02:50Z

Force pushed because of lint in main branch.

sosukesuzuki

Looks good, please check one comment. And please add changelog.

src/cli/format.js

darekkay

I think this PR is just a workaround. ESLint handles this case correctly without deactivating the cache.

src/cli/format.js

tests/integration/run-prettier.js

src/cli/format-results-cache.js

fisker

Thanks for working on this.

tksst · 2022-08-14T08:36:39Z

The current code still does not work properly in the case that multiple files are edited at the same time and one or more files are updated in a formatted state.

$ rm -rf node_modules/.cache
# clear cache

$ node bin/prettier.js --cache --check a.js b.js
Checking formatting...
All matched files use Prettier code style!
# Confirm that these are currently formatted

$ echo "   " >> a.js
# modify to unformatted.

$ echo "const x = 1;" >> b.js
# modify but still formatted.

$ node bin/prettier.js --cache --check a.js b.js
Checking formatting...
[warn] a.js
[warn] Code style issues found in the above file. Forgot to run Prettier?

$ node bin/prettier.js --cache --write a.js b.js
a.js 0ms (cached)
b.js 1ms (cached)
# a.js was cached as formatted!

Not only setFormatResultsCache but also existsAvailableFormatResultsCache cache a current state of file.
I would like to suggest a simple patch to remove cache entries for unformatted files.

fisker · 2022-09-28T17:25:25Z

tests/integration/__tests__/cache.js

    });

+    it("re-formats after execution without write.", async () => {
+      await runPrettier(dir, ["--cache", "--cache-strategy", "metadata", "."]);


Let's put this call in a function, so we don't need check two places when updating.

Maybe even better to accept times as a argument.

Or simply reuse an array to store cli arguments.

@fisker I've added getCliArguments util function. What do you think about d653fe1 ?

It's unreadable... I only ask to make it easier to maintain the second format arguments by putting them together.

Like this?: 9406d54

bryanjtc · 2022-10-22T06:02:41Z

Update on this?

Refactor

This reverts commit 18be64b.

This reverts commit d653fe1.

fisker · 2022-10-23T09:25:37Z

Feel this will be hard to fix conflicts when merge to next branch. 😢

sosukesuzuki · 2022-10-24T14:55:32Z

Feel this will be hard to fix conflicts when merge to next branch. 😢

Yes..., I'll do merge main to next after merging this

sosukesuzuki · 2022-10-25T01:58:14Z

@Milly Thank you for doing this! Good job!

fisker · 2022-10-25T02:08:03Z

@sosukesuzuki Good job merging this into next!

* Add missing changelog * Fix

* 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]>

pspeter3 · 2022-11-29T17:49:21Z

Will --check use the cache?

prettier/prettier#13016

* Fixed typo * Return early if file does not exist * Added tests for cache * Added `mockWriteFileErrors` option to run helper * Added tests for write error * Remove files with differences from cache * Add `getCliArguments` utility function Refactor * Fix lint * Revert "Fix lint" This reverts commit 18be64b. * Revert "Add `getCliArguments` utility function" This reverts commit d653fe1. * Refactor with `cliArguments` variable Co-authored-by: Sosuke Suzuki <[email protected]>

* Add missing changelog * Fix

* 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]>

sosukesuzuki self-requested a review June 18, 2022 12:50

Milly force-pushed the cache-only-write-or-no-different branch from 3f252a7 to 4d97fb7 Compare June 20, 2022 08:59

sosukesuzuki reviewed Jun 26, 2022

View reviewed changes

src/cli/format.js Outdated Show resolved Hide resolved

darekkay reviewed Jun 28, 2022

View reviewed changes

Milly force-pushed the cache-only-write-or-no-different branch from 1fb45b9 to b0a5828 Compare June 29, 2022 11:28

sosukesuzuki approved these changes Jul 31, 2022

View reviewed changes

sosukesuzuki requested a review from fisker July 31, 2022 14:55

fisker reviewed Aug 1, 2022

View reviewed changes

src/cli/format.js Show resolved Hide resolved

fisker reviewed Aug 1, 2022

View reviewed changes

tests/integration/run-prettier.js Outdated Show resolved Hide resolved

fisker reviewed Aug 1, 2022

View reviewed changes

tests/integration/run-prettier.js Outdated Show resolved Hide resolved

fisker reviewed Aug 1, 2022

View reviewed changes

src/cli/format-results-cache.js Show resolved Hide resolved

fisker approved these changes Aug 1, 2022

View reviewed changes

Milly requested a review from darekkay August 1, 2022 18:57

Milly added 6 commits August 18, 2022 07:35

Fixed typo

d5336d2

Return early if file does not exist

47d4767

Added tests for cache

1a78d53

Added mockWriteFileErrors option to run helper

3c4f3c4

Added tests for write error

b7451e2

Remove files with differences from cache

4dbd265

fisker reviewed Sep 28, 2022

View reviewed changes

takonasu mentioned this pull request Oct 7, 2022

Pretter を 2.7 に twin-te/twinte-front#554

Open

HolgerJeromin mentioned this pull request Oct 22, 2022

node_modules/.cache/prettier #13032

Closed

Sosuke Suzuki added 5 commits October 23, 2022 14:47

Add getCliArguments utility function

d653fe1

Refactor

Fix lint

18be64b

Revert "Fix lint"

584bd19

This reverts commit 18be64b.

Revert "Add getCliArguments utility function"

915cca1

This reverts commit d653fe1.

Refactor with cliArguments variable

9406d54

fisker approved these changes Oct 23, 2022

View reviewed changes

sosukesuzuki merged commit ca246af into prettier:main Oct 24, 2022

Milly deleted the cache-only-write-or-no-different branch October 24, 2022 22:02

sosukesuzuki mentioned this pull request Nov 6, 2022

Add missing changelog for #13016 #13812

Merged

4 tasks

sosukesuzuki pushed a commit that referenced this pull request Nov 6, 2022

Add missing changelog for #13016 (#13812)

a980caa

* Add missing changelog * Fix

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

Reword changelog for prettier#13016

cd1540a

thorn0 mentioned this pull request Nov 22, 2022

2.8 blog post #13813

Merged

1 task

HiDeoo added a commit to HiDeoo/create-app that referenced this pull request Jan 10, 2023

feat: use prettier cache on npm lint script

3068c07

prettier/prettier#13016

HiDeoo mentioned this pull request Jan 10, 2023

Use Prettier cache on npm lint script HiDeoo/create-app#20

Merged

falsepopsky mentioned this pull request Apr 26, 2023

chore(deps): bump and fix prettier carbon-design-system/carbon#13675

Merged

medikoo pushed a commit to medikoo/prettier-elastic that referenced this pull request Jan 4, 2024

Add missing changelog for prettier#13016 (prettier#13812)

5ab6bff

* Add missing changelog * Fix

pkong-ds mentioned this pull request Aug 8, 2024

Do not cache prettier:format in portal authgear/authgear-server#4551

Merged

Uh oh!

Conversation

Milly commented Jun 16, 2022

Description

Before fixed:

After fixed with this PR:

Checklist

Uh oh!

fisker commented Jun 16, 2022

Uh oh!

Milly commented Jun 16, 2022

Conditions to be cached

Uh oh!

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

Uh oh!

Uh oh!

fisker commented Jun 16, 2022

Uh oh!

Milly commented Jun 16, 2022

Uh oh!

Milly commented Jun 17, 2022

Uh oh!

Milly commented Jun 17, 2022

Uh oh!

Milly commented Jun 17, 2022

Uh oh!

Milly commented Jun 17, 2022

Uh oh!

Milly commented Jun 20, 2022

Uh oh!

sosukesuzuki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

darekkay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fisker left a comment

Choose a reason for hiding this comment

Uh oh!

tksst commented Aug 14, 2022

Uh oh!

fisker Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

fisker Sep 28, 2022

Choose a reason for hiding this comment

Uh oh!

sosukesuzuki Oct 23, 2022

Choose a reason for hiding this comment

Uh oh!

fisker Oct 23, 2022

Choose a reason for hiding this comment

Uh oh!

sosukesuzuki Oct 23, 2022

Choose a reason for hiding this comment

Uh oh!

fisker Oct 23, 2022

Choose a reason for hiding this comment

Uh oh!

bryanjtc commented Oct 22, 2022

Uh oh!

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

Uh oh!

Uh oh!

sosukesuzuki commented Oct 24, 2022

Uh oh!

sosukesuzuki commented Oct 25, 2022

Uh oh!

fisker commented Oct 25, 2022

Uh oh!

pspeter3 commented Nov 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Milly commented Jun 16, 2022 •

edited

Loading

fisker commented Oct 23, 2022 •

edited

Loading