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

Log after subsequent compilations in --watch mode #11220

Merged
merged 4 commits into from May 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
69 changes: 55 additions & 14 deletions packages/babel-cli/src/babel/dir.js
@@ -1,6 +1,7 @@
// @flow

import defaults from "lodash/defaults";
import debounce from "lodash/debounce";
import { sync as makeDirSync } from "make-dir";
import slash from "slash";
import path from "path";
Expand Down Expand Up @@ -138,24 +139,50 @@ export default async function({
}
}

let compiledFiles = 0;
let startTime = null;

const logSuccess = debounce(
function() {
if (startTime === null) {
// This should never happen, but just in case it's better
// to ignore the log message rather than making @babel/cli crash.
return;
}

const diff = process.hrtime(startTime);

console.log(
`Successfully compiled ${compiledFiles} ${
compiledFiles !== 1 ? "files" : "file"
} with Babel (${diff[0] * 1e3 + Math.round(diff[1] / 1e6)}ms).`,
);
compiledFiles = 0;
JLHwung marked this conversation as resolved.
Show resolved Hide resolved
startTime = null;
},
100,
{ trailing: true },
);

if (!cliOptions.skipInitialBuild) {
if (cliOptions.deleteDirOnStart) {
util.deleteDir(cliOptions.outDir);
}

makeDirSync(cliOptions.outDir);

let compiledFiles = 0;
startTime = process.hrtime();

for (const filename of cliOptions.filenames) {
// compiledFiles is just incremented without reading its value, so we
// don't risk race conditions.
// eslint-disable-next-line require-atomic-updates
compiledFiles += await handle(filename);
}

if (!cliOptions.quiet) {
console.log(
`Successfully compiled ${compiledFiles} ${
compiledFiles !== 1 ? "files" : "file"
} with Babel.`,
);
logSuccess();
logSuccess.flush();
}
}

Expand All @@ -172,16 +199,30 @@ export default async function({
},
});

// This, alongside with debounce, allows us to only log
// when we are sure that all the files have been compiled.
let processing = 0;

["add", "change"].forEach(function(type: string): void {
watcher.on(type, function(filename: string): void {
handleFile(
filename,
filename === filenameOrDir
? path.dirname(filenameOrDir)
: filenameOrDir,
).catch(err => {
watcher.on(type, async function(filename: string) {
processing++;
if (startTime === null) startTime = process.hrtime();

try {
await handleFile(
filename,
filename === filenameOrDir
? path.dirname(filenameOrDir)
: filenameOrDir,
);

compiledFiles++;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will compiledFiles be incremented twice if a single file is modified twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

With the current implementation, if it is modified twice in a very short period of time, yes.

Do you think that the memory/complexity of using a Set with the file paths instead of the simple counter? (This may look like a rhetorical question, but it's not 😅)

Copy link
Contributor

@JLHwung JLHwung Mar 11, 2020

Choose a reason for hiding this comment

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

On devtools like Babel I prefer correctness over performance unless it becomes our bottleneck.

On the other hand, thinking of the DX, does Successfully compiled # files with babel really deliver value? I mean as a developer I would not care how many files are compiled, especially when in watch mode. In Babel repo we are printing the compiled path, execution timestamp in watch mode. If one is changing a single file using babel-cli, it will keep printing successfully compiled 1 file with Babel so babel-cli can not assure users that the file has been compiled after they saved it.

What about printing things like "successfully compiled after 346ms", not ideal but at least they are more likely to differ across different runs.

I am okay to leave it as-is and address this in a separate DX PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'm up for adjusting the message too, in a follow-up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

It now logs

Successfully compiled 3 files with Babel (123ms).

} catch (err) {
console.error(err);
});
}

processing--;
if (processing === 0 && !cliOptions.quiet) logSuccess();
});
});
});
Expand Down
Expand Up @@ -2,4 +2,4 @@

arr.map(function (x) {
return x * MULTIPLIER;
});
});
@@ -1,2 +1,2 @@
src/index.js -> lib/index.js
Successfully compiled 1 file with Babel.
Successfully compiled 1 file with Babel (123ms).
@@ -1,2 +1,2 @@
src/index.js -> lib/index.js
Successfully compiled 1 file with Babel.
Successfully compiled 1 file with Babel (123ms).
@@ -1,3 +1,3 @@
src/foo/.foo.js -> lib/foo/.foo.js
src/foo/bar.js -> lib/foo/bar.js
Successfully compiled 2 files with Babel.
Successfully compiled 2 files with Babel (123ms).
@@ -1,4 +1,4 @@
src/.foo.js -> lib/.foo.js
src/bar/index.js -> lib/bar/index.js
src/foo/foo.js -> lib/foo/foo.js
Successfully compiled 3 files with Babel.
Successfully compiled 3 files with Babel (123ms).
@@ -1,2 +1,2 @@
src/index.js -> lib/index.js
Successfully compiled 1 file with Babel.
Successfully compiled 1 file with Babel (123ms).
@@ -1,2 +1,2 @@
src/index.js -> lib/index.js
Successfully compiled 1 file with Babel.
Successfully compiled 1 file with Babel (123ms).
@@ -1,2 +1,2 @@
src/index.js -> lib/index.js
Successfully compiled 1 file with Babel.
Successfully compiled 1 file with Babel (123ms).
@@ -1,2 +1,2 @@
src/foo/bar.js -> lib/foo/bar.js
Successfully compiled 1 file with Babel.
Successfully compiled 1 file with Babel (123ms).
@@ -1,2 +1,2 @@
src/index.js -> lib/index.js
Successfully compiled 1 file with Babel.
Successfully compiled 1 file with Babel (123ms).
@@ -1,2 +1,2 @@
src/index.js -> lib/index.js
Successfully compiled 1 file with Babel.
Successfully compiled 1 file with Babel (123ms).
@@ -1,2 +1,2 @@
src/index.js -> lib/index.js
Successfully compiled 1 file with Babel.
Successfully compiled 1 file with Babel (123ms).
@@ -1,2 +1,2 @@
src/foo/bar.js -> lib/foo/bar.js
Successfully compiled 1 file with Babel.
Successfully compiled 1 file with Babel (123ms).
@@ -1,3 +1,3 @@
src/bar/index.js -> lib/bar/index.js
src/foo/foo.js -> lib/foo/foo.js
Successfully compiled 2 files with Babel.
Successfully compiled 2 files with Babel (123ms).
@@ -1,2 +1,2 @@
src/foobar/foo.js -> lib/foobar/foo.js
Successfully compiled 1 file with Babel.
Successfully compiled 1 file with Babel (123ms).
Expand Up @@ -2,4 +2,4 @@ src/a.js -> lib/a.js
src/b.js -> lib/b.js
src/baz/c.js -> lib/baz/c.js
src/foo.js -> lib/foo.js
Successfully compiled 4 files with Babel.
Successfully compiled 4 files with Babel (123ms).
2 changes: 1 addition & 1 deletion packages/babel-cli/test/fixtures/babel/--ignore/stdout.txt
@@ -1,2 +1,2 @@
src/bar/index.js -> lib/bar/index.js
Successfully compiled 1 file with Babel.
Successfully compiled 1 file with Babel (123ms).
@@ -1,3 +1,3 @@
src/a.foo.js -> lib/a.foo.js
src/baz/b.foo.js -> lib/baz/b.foo.js
Successfully compiled 2 files with Babel.
Successfully compiled 2 files with Babel (123ms).
2 changes: 1 addition & 1 deletion packages/babel-cli/test/fixtures/babel/--only/stdout.txt
@@ -1,2 +1,2 @@
src/bar/index.js -> lib/bar/index.js
Successfully compiled 1 file with Babel.
Successfully compiled 1 file with Babel (123ms).
@@ -1,2 +1,2 @@
src/foo.js -> lib/foo.js
Successfully compiled 1 file with Babel.
Successfully compiled 1 file with Babel (123ms).
@@ -1,3 +1,3 @@
src/bar.mjs -> lib/bar.mjs
src/foo.js -> lib/foo.js
Successfully compiled 2 files with Babel.
Successfully compiled 2 files with Babel (123ms).
@@ -1,3 +1,3 @@
src/bar.mjs -> lib/bar.mjs
src/foo.jsx -> lib/foo.mjs
Successfully compiled 2 files with Babel.
Successfully compiled 2 files with Babel (123ms).
Expand Up @@ -2,4 +2,4 @@ package1/src/bar/bar1.js -> package1/lib/bar/bar1.js
package1/src/foo1.js -> package1/lib/foo1.js
package2/src/bar/bar2.js -> package2/lib/bar/bar2.js
package2/src/foo2.js -> package2/lib/foo2.js
Successfully compiled 4 files with Babel.
Successfully compiled 4 files with Babel (123ms).
@@ -1,3 +1,3 @@
src/bar/bar.js -> lib/bar/bar.js
src/foo.js -> lib/foo.js
Successfully compiled 2 files with Babel.
Successfully compiled 2 files with Babel (123ms).
@@ -1,3 +1,3 @@
src/bar/bar.js -> lib/bar/bar.js
src/foo.js -> lib/foo.js
Successfully compiled 2 files with Babel.
Successfully compiled 2 files with Babel (123ms).
@@ -1,3 +1,3 @@
src/bar/bar.js -> lib/bar/bar.js
src/foo.js -> lib/foo.js
Successfully compiled 2 files with Babel.
Successfully compiled 2 files with Babel (123ms).
@@ -1 +1 @@
Successfully compiled 2 files with Babel.
Successfully compiled 2 files with Babel (123ms).
@@ -1 +1 @@
Successfully compiled 0 files with Babel.
Successfully compiled 0 files with Babel (123ms).
@@ -1,2 +1,2 @@
src/foo.js -> lib/foo.js
Successfully compiled 1 file with Babel.
Successfully compiled 1 file with Babel (123ms).
@@ -1,2 +1,2 @@
src/foo.js -> lib/foo.js
Successfully compiled 1 file with Babel.
Successfully compiled 1 file with Babel (123ms).
10 changes: 6 additions & 4 deletions packages/babel-cli/test/index.js
Expand Up @@ -49,19 +49,19 @@ const saveInFiles = function(files) {
});
};

const replacePaths = function(str, cwd) {
const normalizeOutput = function(str, cwd) {
let prev;
do {
prev = str;
str = str.replace(cwd, "<CWD>");
} while (str !== prev);

return str;
return str.replace(/\(\d+ms\)/g, "(123ms)");
};

const assertTest = function(stdout, stderr, opts, cwd) {
stdout = replacePaths(stdout, cwd);
stderr = replacePaths(stderr, cwd);
stdout = normalizeOutput(stdout, cwd);
stderr = normalizeOutput(stderr, cwd);

const expectStderr = opts.stderr.trim();
stderr = stderr.trim();
Expand All @@ -84,6 +84,7 @@ const assertTest = function(stdout, stderr, opts, cwd) {
if (opts.stdoutContains) {
expect(stdout).toContain(expectStdout);
} else {
fs.writeFileSync(opts.stdoutPath, stdout + "\n");
expect(stdout).toBe(expectStdout);
}
} else if (stdout) {
Expand Down Expand Up @@ -230,6 +231,7 @@ fs.readdirSync(fixtureLoc).forEach(function(binName) {

["stdout", "stdin", "stderr"].forEach(function(key) {
const loc = path.join(testLoc, key + ".txt");
opts[key + "Path"] = loc;
if (fs.existsSync(loc)) {
opts[key] = helper.readFile(loc);
} else {
Expand Down