Add --only-changed flag to CLI #5910

g-harel · 2019-02-26T03:38:57Z

Add an --only-changed flag to be used in conjunction with --write to avoid re-checking files that were not changed since they were last written (with the same formatting configuration). These results are stored in the .prettiercache file by default, but this can be configured using the PRETTIER_CACHE_LOCATION environment variable.

prettier --write --only-changed ...

Here are some rough manual benchmarks using

time ./bin/prettier.js --write --only-changed "**/*.js"

	prettier source	~300 file project
write	31s	8s
only-changed initial	31s	8s
only-changed 2nd run	8s	2s

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 not an internal change) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
I’ve read the contributing guidelines.

✨Try the playground for this PR✨

src/cli/changed-cache.js

g-harel · 2019-03-02T21:54:46Z

@lydell

~~I'm still missing some coverage % for the error handling in the changed cache implementation. I'd like to add~~ I had to add some unit tests, but I can't find an appropriate place to put them!

mbrookes · 2019-03-06T23:11:37Z

These results are stored in the .prettiercache file by default

@g-harel Going back to the discussion you linked to in the related issue... Even though it seems no consensus was reached, what do you think about using .cache/prettier? Prettier is ubiquitous enough that it could help set the standard, and if not, it's the same number of entries (one!) to add to .gitignore.

(Similarly I'd love do see tools adopt .config/toolname as the default configuration file location, rather than littering the root dir, but one step at a time!)

g-harel · 2019-03-07T22:09:42Z

@mbrookes

Absolutely, I only used .prettiercache to match the pattern from eslint.

Do you know who would need to make the final call?

lydell · 2019-03-08T19:10:32Z

.cache/ might not be a good idea, because apparently it's used by Parcel:

https://github.com/github/gitignore/blob/37abd930a6893fd3a716c48934e704795bc2a45c/Node.gitignore#L61-L62

mbrookes · 2019-03-08T21:22:36Z

apparently it's used by Parcel:

@lydell Great! The whole idea is that it's a single ubiquitous directory to be shared by any tool or utility that wishes to cache some data, rather than polluting the root directory with multiple .xyzcache files.

g-harel · 2019-03-08T23:43:19Z

@mbrookes From what I remember using parcel, it doesn't use the folder in the way you outlined. It uses the entire folder as a cache and creates sub-directories with hex filenames to store the data.

It looks like the two would not interfere, but given it's parcel's cache file, it might be best to avoid it.

mbrookes · 2019-03-09T10:51:09Z

Personally I think it’s a missed opportunity. It would be naive of the Parcel devs to assume that a directory called .cache would be exclusive to Parcel. However, if someone is using both Parcel and prettier and it does cause problems, both tools have an option change the cache directory until Parcel is fixed.

azz · 2019-03-10T10:14:04Z

Just playing devil's advocate on this one, if this is a CLI-only change, wouldn't it be better to have an external CLI tool that conditionally operates on files?

e.g. say we had a CLI tool called smart-exec, that looked something like:

$ smart-exec "**/*.js" -- prettier --write

This would check a local cache before invoking the command after -- for each file.

You could even wire it up to .prettierignore

$ smart-exec "**/*.js" --ignore-path=.prettierignore -- prettier --write

There's nothing Prettier-specific about the implementation, so it should be perfectly re-usable.

Alternatively, if you wanted to do a bespoke prettier-cache tool, you could go zero-config ✨:

$ prettier-changed

This would behave exactly the same as the previous command, you could even build it on top of proposed smart-exec.

I worry about adding too much frills to prettier-core (it's why pretty-quick is a standalone tool). UNIX philosophy.

g-harel · 2019-03-15T21:29:29Z

I think this feature is important enough on large projects to be included with the "core" CLI.

The current approach (in this PR) can be abstracted to a different tool, but it might make sense to eventually extend caching behavior (ex. --check).

I feel prettier-quick's dependency on external tools (git, svn) puts it in a different league because of the increased complexity and required environment configuration.

g-harel · 2019-04-09T17:31:55Z

Any updates on this? What is the decision making process?

src/cli/util.js

alexander-akait · 2019-04-09T17:41:38Z

src/cli/index.js

+    if (context.argv["only-changed"] && !context.argv["write"]) {
+      context.logger.error("Cannot use --only-changed without --write.");
+      process.exit(1);
+    }


Maybe we should enable cache by default and better add option --no-cache (better DX, this do many tools. example jest)

I thought it might be best to avoid changing existing behavior silently, especially since this flag writes a file that needs to gitignored. I'll defer to the maintainers, y'all know best.

/cc @prettier/core

Maybe we can do this for v2

Yeah, probably better to do it for v2

But what is problem implement --no-cache by default?

src/cli/changed-cache.js

alexander-akait · 2019-04-09T17:44:06Z

src/cli/changed-cache.js

+  return crypto
+    .createHash("sha1")
+    .update(data)
+    .digest("base64");


Maybe md4 (better use xxHash, but xxHash doesn't built-in in node 😞 ), sha1 is very slow, also we don't need strong here

We had a bit of a discussion about this on the issue and ended up finding that hashing is not a bottleneck in this case. I'm open to changing it, but I'm not sure it's worth adding a dependency.

#5853 (comment)

Okey, let's wait other feedback, for me md4 is best solution here, when you have really big codebase, it is increase perf around 5%

alexander-akait

Good idea, some comments

JelteF · 2019-04-30T15:23:15Z

This looks like a really nice addition. Would this also work when using --list-different instead of --write? i.e. it would only check the files not checked by the last --write

Co-Authored-By: g-harel <gabrielj.harel@gmail.com>

g-harel · 2019-05-04T23:50:29Z

@evilebottnawi

Sorry about the delay, I was in exam season.

I've changed the cache directory and switched to write-file-atomic. Let me know what you think of the changes!

chdsbd · 2019-05-25T17:52:04Z

This is great addition to prettier. Thank you for taking the time to build this feature.

I found myself wondering why python/black was so much faster than Prettier and it's because Prettier doesn't have this feature yet.

What needs to be done before this PR can get merged?

g-harel · 2019-06-06T00:08:32Z

@chdsbd

There is a discussion left unresolved about potentially having this behavior be enabled by default, which would need to be introduced in the next major version bump.

@evilebottnawi @lydell

Anything I can do to move this along?

mbrookes · 2019-06-06T00:13:48Z

Anything I can do to move this along?

Storm prettier HQ and merge it yourself? 😆

lydell

I like this the way it is! Good job!

duailibe · 2019-06-07T22:54:20Z

tests_integration/__tests__/only-changed.js

+  expect(cacheContentsAfter).not.toBe(cacheContentsBefore);
+});
+
+describe("ChangedCache", () => {


We don't have much of those unit tests in the repo, but this is fine

duailibe · 2019-06-07T22:55:21Z

@g-harel Thanks for pushing this through. This will go out on the next minor release, so I'll wait a bit until 1.18 gets more stable, so we don't have to push any other patches

Also don't worry about updating the branch, I'll take care of any conflicts before merge

alexander-akait · 2019-07-22T14:20:53Z

src/cli/util.js

@@ -441,6 +443,18 @@ function formatFiles(context) {
    context.logger.log("Checking formatting...");
  }

+  let changedCache = null;
+  if (context.argv["only-changed"]) {
+    const cacheDir = findCacheDir({ name: "prettier", create: true });


/cc @g-harel need use || os.tmp() https://github.com/avajs/find-cache-dir#findcachediroptions

It returns a string containing the absolute path to the cache directory, or undefined if package.json was never found.

In theory you can install prettier globally and doesn't have package.json

Nice catch! I've openned a PR with a fix (#6327).

g-harel mentioned this pull request Feb 26, 2019

Add cache option to avoid processing unchanged files #5853

Closed

lydell reviewed Feb 26, 2019

View reviewed changes

src/cli/changed-cache.js Outdated Show resolved Hide resolved

g-harel force-pushed the only-changed branch from aa30530 to 4a5f5c7 Compare March 4, 2019 23:49

g-harel marked this pull request as ready for review March 5, 2019 21:15

alexander-akait reviewed Apr 9, 2019

View reviewed changes

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

alexander-akait reviewed Apr 9, 2019

View reviewed changes

src/cli/changed-cache.js Outdated Show resolved Hide resolved

alexander-akait reviewed Apr 9, 2019

View reviewed changes

g-harel force-pushed the only-changed branch from cbcdc26 to c3ad327 Compare May 4, 2019 17:02

g-harel and others added 9 commits May 4, 2019 13:02

add initial implementation of mtime based cache prettier#5853

aa67512

make cache sensitive to option changes prettier#5853

7aa5f3b

update test snapshots

9d04d64

update serialization error message

d488c24

Co-Authored-By: g-harel <gabrielj.harel@gmail.com>

start snapshot tests prettier#5853

66e21f6

update docs prettier#5853

1952ba9

refactor changed-cache into class prettier#5853

91951df

add virtualFile integration test option

8679b70

add tests for unchanged files and config changes prettier#5853

47ffe28

g-harel added 2 commits May 4, 2019 13:02

add tests for changed-cache error handling prettier#5853

12c1b77

add changelog entry prettier#5853

392c451

g-harel force-pushed the only-changed branch from c3ad327 to 392c451 Compare May 4, 2019 17:09

modify changed-cache path + atomic cache writes prettier#5853

94f3c2b

g-harel force-pushed the only-changed branch from c89110e to 8a4733a Compare May 4, 2019 18:59

use node v4 compatible packages prettier#5853

7420a3b

g-harel force-pushed the only-changed branch 2 times, most recently from b0e1335 to a5e9f3f Compare May 4, 2019 20:34

fix mocks prettier#5853

8f2fa71

g-harel force-pushed the only-changed branch from a5e9f3f to 8f2fa71 Compare May 4, 2019 20:54

fix implementation for node v4 prettier#5853

126eedf

lydell approved these changes Jun 7, 2019

View reviewed changes

g-harel added 2 commits June 7, 2019 13:52

Merge branch 'master' into only-changed

171603a

Merge branch 'master' into only-changed

ddeaadc

duailibe approved these changes Jun 7, 2019

View reviewed changes

g-harel added 3 commits June 16, 2019 11:53

Merge branch 'master' into only-changed

c9add14

Merge branch 'master' into only-changed

db47a2a

Merge branch 'master' into only-changed

5e1e508

j-f1 approved these changes Jul 15, 2019

View reviewed changes

duailibe merged commit 6fae09b into prettier:master Jul 22, 2019

alexander-akait reviewed Jul 22, 2019

View reviewed changes

g-harel mentioned this pull request Jul 23, 2019

Handle missing cache directory for changedCache #6327

Merged

4 tasks

alexander-akait mentioned this pull request Sep 30, 2019

--only-changed need review again #6577

Closed

lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Oct 21, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 21, 2019

Uh oh!

Add --only-changed flag to CLI #5910

Add --only-changed flag to CLI #5910

Uh oh!

Conversation

g-harel commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

g-harel commented Mar 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbrookes commented Mar 6, 2019

Uh oh!

g-harel commented Mar 7, 2019

Uh oh!

lydell commented Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbrookes commented Mar 8, 2019

Uh oh!

g-harel commented Mar 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mbrookes commented Mar 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azz commented Mar 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

g-harel commented Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

g-harel commented Apr 9, 2019

Uh oh!

Uh oh!

alexander-akait Apr 9, 2019 • 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!

Uh oh!

alexander-akait Apr 9, 2019 • 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!

alexander-akait left a comment • 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!

JelteF commented Apr 30, 2019

Uh oh!

g-harel commented May 4, 2019

Uh oh!

chdsbd commented May 25, 2019

Uh oh!

g-harel commented Jun 6, 2019

Uh oh!

mbrookes commented Jun 6, 2019

Uh oh!

lydell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duailibe commented Jun 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

g-harel commented Feb 26, 2019 •

edited

Loading

g-harel commented Mar 2, 2019 •

edited

Loading

lydell commented Mar 8, 2019 •

edited

Loading

g-harel commented Mar 8, 2019 •

edited

Loading

mbrookes commented Mar 9, 2019 •

edited

Loading

azz commented Mar 10, 2019 •

edited

Loading

g-harel commented Mar 15, 2019 •

edited

Loading

alexander-akait Apr 9, 2019 •

edited

Loading

alexander-akait Apr 9, 2019 •

edited

Loading

alexander-akait left a comment •

edited

Loading

duailibe commented Jun 7, 2019 •

edited

Loading

g-harel Jul 23, 2019 •

edited

Loading