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

cli: handle multiple input sources in watch mode #14281

Merged
merged 4 commits into from Feb 21, 2022

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Feb 16, 2022

Q                       A
Fixed Issues? Fixes #14269
Patch: Bug Fix? Y
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

In this PR we fix a regression introduced in #14065. Currently we registered multiple listeners for each --src parameter, however, each listeners will be invoked unconditionally even only one of file specified by some --src parameter are changed. Therefore babel-cli creates multiple compilation and, which is worse, outputs compiled files on unwanted paths.

In this PR we determine the input base from the filename when multiple --src parameters are specified. A cache is introduced to avoid repeatedly searching --src parameters.

@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: cli labels Feb 16, 2022
@babel-bot
Copy link
Collaborator

babel-bot commented Feb 16, 2022

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51335/


setTimeout(() => {
console.error("EXECUTOR TIMEOUT");
process.exit(1);
Copy link
Member

Choose a reason for hiding this comment

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

We could replace exit(1) with exit(0) in all the watch tests and use .log instead of .error, to see at which point they get stuck.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! I can't reproduce the timeout error on my local machine, now I have to abuse GitHub CI to see what happens.

let files = [yield, yield].sort();
assert.match(files[0], /src[\\/]index.js -> lib[\\/]index.js/);
assert.match(files[1], /src[\\/]main.js -> lib[\\/]main.js/);
assert.match(yield, /Successfully compiled 2 files with Babel \(\d+ms\)\./);

logFile("lib/index.js");
logFile("lib/main.js");

// wait 200ms for watcher setup
await new Promise(resolve => setTimeout(resolve, 200));
Copy link
Member

Choose a reason for hiding this comment

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

Ohhh thanks for this!

// A map from absolute compiled file path to its base, from which
// the output destination will be determined
const filenameToBaseMap: Map<string, string> = new Map(
filenames.map(filename => {
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering whether these could be globs or not, but they are already resolved at

let filenames = commander.args.reduce(function (globbed, input) {
let files = glob.sync(input);
if (!files.length) files = [input];
globbed.push(...files);
return globbed;
}, []);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes they are resolved prior to watch setup, which means if you run babel */src --watch and then create a new folder module3/src, it will not be added to the watcher.

@JLHwung
Copy link
Contributor Author

JLHwung commented Feb 18, 2022

Interesting, tests are still failing even if I invoke initial build after the watcher claims ready: https://github.com/babel/babel/actions/runs/1866868095 I have revert this commit and will explore it in another PR.

What a flaky test. Test is passing on my fork: https://github.com/JLHwung/babel/runs/5254435895

@nicolo-ribaudo nicolo-ribaudo merged commit c4dc52e into babel:main Feb 21, 2022
@nicolo-ribaudo nicolo-ribaudo deleted the fix-14269 branch February 21, 2022 22:20
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 24, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
i: regression outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: cli PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Babel watch is broken for multiple subdirectories on 7.17.X
4 participants