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: update transform error message #14139

Merged
merged 6 commits into from
Oct 9, 2023

Conversation

btea
Copy link
Collaborator

@btea btea commented Aug 17, 2023

Description

Related #13687

image

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 Aug 17, 2023

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

@btea
Copy link
Collaborator Author

btea commented Sep 16, 2023

@bluwy Please take a look when you are free, thank you!

@bluwy
Copy link
Member

bluwy commented Oct 9, 2023

Sorry I meant prepending the 'Pre-transform error: ' string at these places instead. I think that helps clarify at which phase the error is coming from. Or am I missing how the error is logged?

config.logger.error(e.message, { error: e })

server.config.logger.error(e.message)

@bluwy bluwy added the p2-nice-to-have Not breaking anything but nice to have (priority) label Oct 9, 2023
@btea
Copy link
Collaborator Author

btea commented Oct 9, 2023

I seem to have misunderstood what you meant. Do you want to concatenate the Pre-transform error: string in front of all error messages, not just EISDIR error messages?

@bluwy
Copy link
Member

bluwy commented Oct 9, 2023

Yeah that's what I mean. Not sure if it's easily reproducible, but do you see 2 different error messages now with this tweak? I think that will hopefully clears up the error message if it ever happen twice again.

@btea
Copy link
Collaborator Author

btea commented Oct 9, 2023

Yeah, after I added the string to importAnalysis.ts, the error message became the following.

image

@bluwy
Copy link
Member

bluwy commented Oct 9, 2023

Thanks! The differentiation between Pre-transform error and Internal server error is nice I think, so we know where the error is happening. IIUC they're separate processes (although the same request), so it's a little hard to deduplicate, but this could be safer for now.

The pre-transform error message could probably look better with the timestamp and [vite] prefix, but that can be improved later on.

bluwy
bluwy previously approved these changes Oct 9, 2023
@btea btea changed the title fix: do not print duplicate error message fix: update transform error message Oct 9, 2023
@btea
Copy link
Collaborator Author

btea commented Oct 9, 2023

The pre-transform error message could probably look better with the timestamp and [vite] prefix, but that can be improved later on.

I've added timestamp option.

@bluwy bluwy merged commit e0eb304 into vitejs:main Oct 9, 2023
10 checks passed
@btea btea deleted the fix/print-duplicate-error-message branch October 9, 2023 12:29
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
None yet
Development

Successfully merging this pull request may close these issues.

3 participants