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

feat: add logger.hasErrorLogged(error) method #3957

Merged
merged 9 commits into from
Aug 4, 2021

Conversation

aleclarson
Copy link
Member

Description

This makes it easier for frameworks which wrap Vite (eg: vite-plugin-ssr) to avoid logging an error that was already logged by Vite.

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 Commit 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.

@aleclarson aleclarson added the p2-nice-to-have Not breaking anything but nice to have (priority) label Jun 25, 2021
@yyx990803
Copy link
Member

Let's pass the error to logger.error as part of the options and do the marking inside logger.error

..and require an `error` option to be defined when `logger.error` is called.

BREAKING CHANGE
@aleclarson
Copy link
Member Author

aleclarson commented Jun 25, 2021

I've added the logger.hasLogged method, and the error option must be defined by callers of logger.error. They can pass error: null if no associated error exists. This ensures the option isn't forgotten about.

That makes this PR a breaking change.

In Vite itself, most logged errors are not rethrown, so marking them is pointless, but I'm passing the Error object whenever possible as a principle.

@aleclarson aleclarson changed the title feat: set loggedByVite on errors logged by Vite feat: add logger.hasLogged(error) method Jun 25, 2021
antfu
antfu previously approved these changes Aug 2, 2021
@antfu antfu changed the title feat: add logger.hasLogged(error) method feat: add logger.hasErrorLogged(error) method Aug 2, 2021
Shinigami92
Shinigami92 previously approved these changes Aug 2, 2021
@antfu antfu dismissed stale reviews from Shinigami92 and themself via d9dbb1d August 2, 2021 12:36
Shinigami92
Shinigami92 previously approved these changes Aug 4, 2021
@patak-dev
Copy link
Member

I see that there is a comment about a breaking change, @antfu is this safe to merge in the 2.5 beta? We'll do a beta-1 today so we could still include this one

antfu
antfu previously approved these changes Aug 4, 2021
Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

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

I have changed the options argument to optional to avoid breaking changes.

@antfu antfu merged commit fb406ce into vitejs:main Aug 4, 2021
aleclarson added a commit to aleclarson/vite that referenced this pull request Sep 30, 2021
Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
aleclarson added a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
@aleclarson aleclarson deleted the feat/loggedByVite branch February 25, 2022 20:03
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.

None yet

5 participants