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

Conversation

nicolo-ribaudo
Copy link
Member

Q                       A
Fixed Issues? Fixes #7926, Closes #6320
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This is a revision of #6320.

  1. I used the same message that we already use for the first compilation
  2. I removed the --noisy option. We already have --quiet and --verbose, and a third option would be too much (unless we implement something like --loglevel). I made it the default behavior since it just replicates what @babel/cli already does.
  3. I used lodash/debounce instead of a custom implementation.

@nicolo-ribaudo nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories pkg: cli labels Mar 6, 2020
: 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).

@babel-bot
Copy link
Collaborator

babel-bot commented Apr 29, 2020

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

Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

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

❤️

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 29, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a7ffc87:

Sandbox Source
quirky-chaum-6ux6q Configuration
eager-rgb-sk350 Configuration

@nicolo-ribaudo nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label May 19, 2020
@nicolo-ribaudo nicolo-ribaudo merged commit 698fe8e into babel:master May 24, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the watch-log branch May 24, 2020 21:03
@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 Aug 24, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: cli PR: New Feature 🚀 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No babel-cli watch mode console output after first compilation
4 participants