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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 11 commits into from Oct 24, 2022
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d5336d2
Fixed typo
Milly Jun 17, 2022
47d4767
Return early if file does not exist
Milly Aug 17, 2022
1a78d53
Added tests for cache
Milly Jun 16, 2022
3c4f3c4
Added `mockWriteFileErrors` option to run helper
Milly Aug 1, 2022
b7451e2
Added tests for write error
Milly Aug 1, 2022
4dbd265
Remove files with differences from cache
Milly Jun 16, 2022
d653fe1
Add `getCliArguments` utility function
sosukesuzuki Oct 23, 2022
18be64b
Fix lint
sosukesuzuki Oct 23, 2022
584bd19
Revert "Fix lint"
sosukesuzuki Oct 23, 2022
915cca1
Revert "Add `getCliArguments` utility function"
sosukesuzuki Oct 23, 2022
9406d54
Refactor with `cliArguments` variable
sosukesuzuki Oct 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 13 additions & 9 deletions src/cli/format-results-cache.js
View file
Open in desktop
Expand Up @@ -40,7 +40,7 @@ function getMetadataFromFileDescriptor(fileDescriptor) {

class FormatResultsCache {
/**
* @param {string} cacheFileLocation The path of cache file location. (default: `node_modules/.cache/prettier/prettier-cache`)
* @param {string} cacheFileLocation The path of cache file location. (default: `node_modules/.cache/prettier/.prettier-cache`)
* @param {string} cacheStrategy
*/
constructor(cacheFileLocation, cacheStrategy) {
Expand All @@ -60,20 +60,17 @@ class FormatResultsCache {
*/
existsAvailableFormatResultsCache(filePath, options) {
const fileDescriptor = this.fileEntryCache.getFileDescriptor(filePath);
const hashOfOptions = getHashOfOptions(options);
const meta = getMetadataFromFileDescriptor(fileDescriptor);
const changed =
fileDescriptor.changed || meta.hashOfOptions !== hashOfOptions;

if (fileDescriptor.notFound) {
return false;
}

if (changed) {
return false;
}
const hashOfOptions = getHashOfOptions(options);
const meta = getMetadataFromFileDescriptor(fileDescriptor);
const changed =
fileDescriptor.changed || meta.hashOfOptions !== hashOfOptions;

return true;
return !changed;
}

/**
Expand All @@ -88,6 +85,13 @@ class FormatResultsCache {
}
}

/**
* @param {string} filePath
*/
removeFormatResultsCache(filePath) {
Milly marked this conversation as resolved.
Show resolved Hide resolved
this.fileEntryCache.removeEntry(filePath);
}

reconcile() {
this.fileEntryCache.reconcile();
}
Expand Down
14 changes: 10 additions & 4 deletions src/cli/format.js
View file
Open in desktop
Expand Up @@ -407,9 +407,8 @@ async function formatFiles(context) {
continue;
}

formatResultsCache?.setFormatResultsCache(filename, options);

const isDifferent = output !== input;
let shouldSetCache = !isDifferent;

if (printedFilename) {
// Remove previously printed filename to log it with duration.
Expand All @@ -433,14 +432,15 @@ async function formatFiles(context) {

try {
await fs.writeFile(filename, output, "utf8");

// Set cache if format succeeds
shouldSetCache = true;
} catch (error) {
/* istanbul ignore next */
context.logger.error(
`Unable to write file: ${filename}\n${error.message}`
);

// Don't exit the process if one file failed
/* istanbul ignore next */
process.exitCode = 2;
}
} else if (!context.argv.check && !context.argv.listDifferent) {
Expand All @@ -462,6 +462,12 @@ async function formatFiles(context) {
writeOutput(context, result, options);
}

if (shouldSetCache) {
formatResultsCache?.setFormatResultsCache(filename, options);
} else {
formatResultsCache?.removeFormatResultsCache(filename);
Milly marked this conversation as resolved.
Show resolved Hide resolved
}

if (isDifferent) {
if (context.argv.check) {
context.logger.warn(filename);
Expand Down
184 changes: 181 additions & 3 deletions tests/integration/__tests__/cache.js
View file
Open in desktop
Expand Up @@ -96,7 +96,7 @@ describe("--cache option", () => {
await expect(fs.stat(defaultCacheFile)).resolves.not.toThrowError();
});

it("does'nt format when cache is available", async () => {
it("doesn't format when cache is available", async () => {
const { stdout: firstStdout } = await runPrettier(dir, [
"--cache",
"--write",
Expand Down Expand Up @@ -227,6 +227,95 @@ describe("--cache option", () => {
);
});

it("re-formats after execution without write.", async () => {
fisker marked this conversation as resolved.
Show resolved Hide resolved
await runPrettier(dir, ["--cache", "--cache-strategy", "metadata", "."]);
Copy link
Sponsor 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
Sponsor 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
Sponsor 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
Sponsor Member

Choose a reason for hiding this comment

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

Yes.


const { stdout: secondStdout } = await runPrettier(dir, [
"--write",
"--cache",
"--cache-strategy",
"metadata",
".",
]);
expect(stripAnsi(secondStdout).split("\n").filter(Boolean)).toEqual(
expect.arrayContaining([
expect.stringMatching(/^a\.js .+ms$/),
expect.stringMatching(/^b\.js .+ms \(cached\)$/),
])
);
});

it("re-formats when multiple cached files are updated.", async () => {
await runPrettier(dir, [
"--write",
"--cache",
"--cache-strategy",
"metadata",
".",
]);

// Update `a.js` to unformatted
await fs.writeFile(path.join(dir, "a.js"), "const a = `a`; ");

// Update `b.js` but still formatted
const time = new Date();
await fs.utimes(path.join(dir, "b.js"), time, time);

await runPrettier(dir, ["--cache", "--cache-strategy", "metadata", "."]);

const { stdout: thirdStdout } = await runPrettier(dir, [
"--write",
"--cache",
"--cache-strategy",
"metadata",
".",
]);
expect(stripAnsi(thirdStdout).split("\n").filter(Boolean)).toEqual(
expect.arrayContaining([
expect.stringMatching(/^a\.js .+ms$/),
expect.stringMatching(/^b\.js .+ms \(cached\)$/),
])
);
});

it("doesn't cache files when write error.", async () => {
const {
stdout: firstStdout,
stderr: firstStderr,
status: firstStatus,
} = await runPrettier(
dir,
["--write", "--cache", "--cache-strategy", "metadata", "."],
{
mockWriteFileErrors: {
"a.js": "EACCES: permission denied (mock error)",
},
}
);
expect(firstStatus).toBe(2);
expect(stripAnsi(firstStderr).split("\n").filter(Boolean)).toEqual([
"[error] Unable to write file: a.js",
"[error] EACCES: permission denied (mock error)",
]);
expect(stripAnsi(firstStdout).split("\n").filter(Boolean)).toEqual(
expect.arrayContaining([
expect.stringMatching(/^a\.js .+ms$/),
expect.stringMatching(/^b\.js .+ms$/),
])
);

const { stdout: secondStdout } = await runPrettier(dir, [
"--list-different",
"--cache",
"--cache-strategy",
"metadata",
".",
]);
expect(stripAnsi(secondStdout).split("\n").filter(Boolean)).toEqual([
"a.js",
]);
});

it("removes cache file when run Prettier without `--cache` option", async () => {
await runPrettier(dir, [
"--cache",
Expand All @@ -251,7 +340,7 @@ describe("--cache option", () => {
await expect(fs.stat(defaultCacheFile)).resolves.not.toThrowError();
});

it("does'nt format when cache is available", async () => {
it("doesn't format when cache is available", async () => {
const { stdout: firstStdout } = await runPrettier(dir, [
"--cache",
"--cache-strategy",
Expand Down Expand Up @@ -315,7 +404,7 @@ describe("--cache option", () => {
);
});

it("does'nt re-format when timestamp has been updated", async () => {
it("doesn't re-format when timestamp has been updated", async () => {
const { stdout: firstStdout } = await runPrettier(dir, [
"--cache",
"--cache-strategy",
Expand Down Expand Up @@ -381,6 +470,95 @@ describe("--cache option", () => {
);
});

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

const { stdout: secondStdout } = await runPrettier(dir, [
"--write",
"--cache",
"--cache-strategy",
"content",
".",
]);
expect(stripAnsi(secondStdout).split("\n").filter(Boolean)).toEqual(
expect.arrayContaining([
expect.stringMatching(/^a\.js .+ms$/),
expect.stringMatching(/^b\.js .+ms \(cached\)$/),
])
);
});

it("re-formats when multiple cached files are updated.", async () => {
await runPrettier(dir, [
"--write",
"--cache",
"--cache-strategy",
"content",
".",
]);

// Update `a.js` to unformatted
await fs.writeFile(path.join(dir, "a.js"), "const a = `a`; ");

// Update `b.js` but still formatted
const time = new Date();
await fs.utimes(path.join(dir, "b.js"), time, time);

await runPrettier(dir, ["--cache", "--cache-strategy", "content", "."]);

const { stdout: thirdStdout } = await runPrettier(dir, [
"--write",
"--cache",
"--cache-strategy",
"content",
".",
]);
expect(stripAnsi(thirdStdout).split("\n").filter(Boolean)).toEqual(
expect.arrayContaining([
expect.stringMatching(/^a\.js .+ms$/),
expect.stringMatching(/^b\.js .+ms \(cached\)$/),
])
);
});

it("doesn't cache files when write error.", async () => {
const {
stdout: firstStdout,
stderr: firstStderr,
status: firstStatus,
} = await runPrettier(
dir,
["--write", "--cache", "--cache-strategy", "content", "."],
{
mockWriteFileErrors: {
"a.js": "EACCES: permission denied (mock error)",
},
}
);
expect(firstStatus).toBe(2);
expect(stripAnsi(firstStderr).split("\n").filter(Boolean)).toEqual([
"[error] Unable to write file: a.js",
"[error] EACCES: permission denied (mock error)",
]);
expect(stripAnsi(firstStdout).split("\n").filter(Boolean)).toEqual(
expect.arrayContaining([
expect.stringMatching(/^a\.js .+ms$/),
expect.stringMatching(/^b\.js .+ms$/),
])
);

const { stdout: secondStdout } = await runPrettier(dir, [
"--list-different",
"--cache",
"--cache-strategy",
"content",
".",
]);
expect(stripAnsi(secondStdout).split("\n").filter(Boolean)).toEqual([
"a.js",
]);
});

it("removes cache file when run Prettier without `--cache` option", async () => {
await runPrettier(dir, ["--cache", "--write", "."]);
await expect(fs.stat(defaultCacheFile)).resolves.not.toThrowError();
Expand Down
4 changes: 4 additions & 0 deletions tests/integration/run-prettier.js
View file
Open in desktop
Expand Up @@ -46,6 +46,10 @@ async function run(dir, args, options) {
.spyOn(fs.promises, "writeFile")
// eslint-disable-next-line require-await
.mockImplementation(async (filename, content) => {
const error = (options.mockWriteFileErrors || {})[filename];
if (error) {
throw new Error(error);
}
write.push({ filename, content });
});

Expand Down