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

Fix --cache set only if with --write or no-different #13016

Merged
merged 11 commits into from Oct 24, 2022

Conversation

Milly
Copy link
Contributor

@Milly Milly commented Jun 16, 2022

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
Copy link
Member

fisker commented Jun 16, 2022

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

@Milly
Copy link
Contributor Author

Milly commented Jun 16, 2022

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
Copy link
Contributor Author

Milly commented Jun 16, 2022

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

@fisker
Copy link
Member

fisker commented Jun 16, 2022

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

@Milly
Copy link
Contributor Author

Milly commented Jun 16, 2022

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

I fixed it.

@Milly
Copy link
Contributor Author

Milly commented Jun 17, 2022

This fixes #13015

@Milly
Copy link
Contributor Author

Milly commented Jun 17, 2022

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
Copy link
Contributor Author

Milly commented Jun 17, 2022

Fixed it.

@Milly
Copy link
Contributor Author

Milly commented Jun 17, 2022

refs #12800

@sosukesuzuki sosukesuzuki self-requested a review Jun 18, 2022
@Milly Milly force-pushed the cache-only-write-or-no-different branch from 3f252a7 to 4d97fb7 Compare Jun 20, 2022
@Milly
Copy link
Contributor Author

Milly commented Jun 20, 2022

Force pushed because of lint in main branch.

Copy link
Member

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

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

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

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

@darekkay darekkay left a comment

Choose a reason for hiding this comment

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

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

@Milly Milly force-pushed the cache-only-write-or-no-different branch from 1fb45b9 to b0a5828 Compare Jun 29, 2022
@sosukesuzuki sosukesuzuki requested a review from fisker Jul 31, 2022
src/cli/format.js Show resolved Hide resolved
tests/integration/run-prettier.js Outdated Show resolved Hide resolved
tests/integration/run-prettier.js Outdated Show resolved Hide resolved
fisker
fisker approved these changes Aug 1, 2022
Copy link
Member

@fisker fisker left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

@Milly Milly requested a review from darekkay Aug 1, 2022
@tksst
Copy link

tksst commented Aug 14, 2022

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
Copy link
Member

fisker commented Sep 28, 2022

close/reopen to trigger ci.

@fisker fisker closed this Sep 28, 2022
@fisker fisker reopened this Sep 28, 2022
@fisker
Copy link
Member

fisker commented Sep 28, 2022

It's back..

@@ -227,6 +227,95 @@ describe("--cache option", () => {
);
});

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

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Or simply reuse an array to store cli arguments.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Like this?: 9406d54

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

@bryanjtc
Copy link

Update on this?

fisker
fisker approved these changes Oct 23, 2022
@fisker
Copy link
Member

fisker commented Oct 23, 2022

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

@sosukesuzuki
Copy link
Member

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 sosukesuzuki merged commit ca246af into prettier:main Oct 24, 2022
28 checks passed
@Milly Milly deleted the cache-only-write-or-no-different branch Oct 24, 2022
@sosukesuzuki
Copy link
Member

@Milly Thank you for doing this! Good job!

@fisker
Copy link
Member

fisker commented Oct 25, 2022

@sosukesuzuki Good job merging this into next!

sosukesuzuki added a commit that referenced this pull request Nov 6, 2022
* Add missing changelog

* Fix
@kachkaev 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
@thorn0 thorn0 mentioned this pull request Nov 22, 2022
1 task
sosukesuzuki added a commit that referenced this pull request Nov 23, 2022
* 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>
@pspeter3
Copy link

Will --check use the cache?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants