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

fix(build): make output warning message clearer #12924

Merged
merged 5 commits into from
Jun 8, 2023

Conversation

btea
Copy link
Collaborator

@btea btea commented Apr 20, 2023

Description

before

a

after

b

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@stackblitz
Copy link

stackblitz bot commented Apr 20, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@ArnaudBarre
Copy link
Member

Related and (maybe fixes) #11207

I don't know the implication but maybe we should clear the line too before calling userOnWarn

@ArnaudBarre ArnaudBarre added the p2-nice-to-have Not breaking anything but nice to have (priority) label Apr 20, 2023
Copy link
Contributor

@danieldeandradelopes danieldeandradelopes left a comment

Choose a reason for hiding this comment

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

Nice.

@bluwy
Copy link
Member

bluwy commented Apr 24, 2023

This could be a bit risky. Sometimes logs appear during transformation, and the logs could give a hint as to which file causes the log (though with an uglier output). This PR would clear the "current transforming file" and loses the information.

@ArnaudBarre
Copy link
Member

Actually this is the opposite because we throttle the displayed file: https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/reporter.ts#L63
So this can avoid displaying the name of an unrelated file before the warning.
But a good point is that we should gives better hints on the module that triggered the warning. Don't know if this should be in the same PR or not

@bluwy
Copy link
Member

bluwy commented Apr 24, 2023

Interesting. I'm not sure why it's being throttled, but I definitely had experienced the file name showed to be accurate that helped me in debugging things.

@ArnaudBarre
Copy link
Member

ArnaudBarre commented Apr 24, 2023

Printing to terminal can be slow, famous bug report: npm/npm#11283

@patak-dev patak-dev added this to the 4.4 milestone May 15, 2023
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Added it to the 4.4 milestone, I think we could merge this one in the next beta to get some folks trying it out.

@bluwy
Copy link
Member

bluwy commented May 16, 2023

I think it would still be good to make this work generically instead of only for this.warn/this.error, since a stray console.log from a plugin would still mess up the logs.

I'd also prefer if we don't clear the current line (even if it could be wrong) since it could be helpful at times.

Perhaps during the transforming log phase, we could capture process.stdout.write and make sure all writes precede with logging the last "transforming id" + a new line + the warning/error logs?

@ArnaudBarre
Copy link
Member

I'm in for monkey patching the console.log or process.write but this can have unexpected interactions with people building on top of it. How does Astro and Nuxt use (or not) the builtin Vite reporter?

The fact that we could log the current transforming file before the log is a big win for me (but does that even possible? Rollup doesn't do parallel execution?)

@patak-dev
Copy link
Member

@bluwy @ArnaudBarre should we move this PR to the Vite 5 milestone?

@ArnaudBarre
Copy link
Member

ArnaudBarre commented Jun 6, 2023

Completely changed my mind: I think we should go with monkey patching directly.
For me it's safe for 4.4, but can be discussed

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

For me, I think it's also fine to merge this PR as a stop gap, and work on a more complete solution later.

Comment on lines 914 to 915
readLine.clearLine(process.stdout, 0)
readLine.cursorTo(process.stdout, 0)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can simplify this like

function clearLine() {
process.stdout.clearLine(0)
process.stdout.cursorTo(0)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@patak-dev patak-dev merged commit 54ab3c8 into vitejs:main Jun 8, 2023
@btea btea deleted the fix/build-warn-output branch June 8, 2023 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants