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: export error message generator #11155

Merged
merged 1 commit into from Dec 7, 2022

Conversation

manucorporat
Copy link
Contributor

@manucorporat manucorporat commented Dec 2, 2022

Description

Exports the buildErrorMessage() util, to be able to implement nice printed error and warning outside the rollup pipeline.

Additional context

Discussion with @patak-dev

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.

@manucorporat manucorporat force-pushed the feat-export-build-error-message branch from a332c02 to cc56bf5 Compare December 2, 2022 14:53
@patak-dev patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label Dec 7, 2022
@bluwy
Copy link
Member

bluwy commented Dec 7, 2022

cc @Princesseuh who has been championing Astro's error handling recently. I'm not sure if this would be useful for Astro but could be interesting for other cases.

@patak-dev
Copy link
Member

For reference, we discussed with @manucorporat about improvements to our error messages that doesn't require exporting this function. But these may take some time to iron out so I think exporting this util is a good move for the mid-term to let frameworks have a consistent error story with Vite.

@bluwy
Copy link
Member

bluwy commented Dec 7, 2022

If the goal is to standardize error message handling, should we also document this in api-javascript so it gets more awareness? I don't have experience with error handling outside of the plugin lifecycle so I don't have much comments on whether this is a good idea or not 😬

@Princesseuh
Copy link
Contributor

Princesseuh commented Dec 7, 2022

cc @Princesseuh who has been championing Astro's error handling recently. I'm not sure if this would be useful for Astro but could be interesting for other cases.

Confirming that for Astro this is not useful, we do all the printing ourselves in the CLI for all kinds of errors. I can see why other frameworks would find it interesting to have though!

@manucorporat
Copy link
Contributor Author

Reason for us is to leverage Vite DX, and not reinvent the well, of course every framework could build their own, but i feel if all efforts go into vite, all of us win.

The reason for this PR is that there are more errors and warnings than crashes or plugin errors.

@patak-dev
Copy link
Member

I think we should merge this. Discussed with Evan too, and he thinks we should be careful moving forward about exporting more utilities. In this case, I agree that for some frameworks this is going to be helpful with a small maintenance cost for us (and Manu is interested in helping on Vite's error story in general, so it is also better that the fixes and improvement go directly to us and not stay downstream).

@patak-dev patak-dev merged commit 493ba1e into vitejs:main Dec 7, 2022
futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
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

4 participants