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(css): enhance error message for missing preprocessor dependency #11485

Merged
merged 7 commits into from
Jul 31, 2023

Conversation

EastSun5566
Copy link
Contributor

@EastSun5566 EastSun5566 commented Dec 25, 2022

Hi Vite team

Thanks for the great tool!

Description

I recently encountered an error while migrating my build tool to Vite. I had forgotten to install the preprocessor dependency. However, I really appreciate the helpful and clear TypeScript diagnostic messages that include instructions on how to fix the issue. I think these messages greatly improve DX, so I decided to add them to it.

What do you think? Thank you for your help!

Additional context

enhance error messages for missing preprocessor dependency by giving instructions like npm i -D sass etc.


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.

packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
@bluwy bluwy changed the title chore(vite): enhance error message when can't find CSS preprocessor dependency fix(css): enhance error message for missing preprocessor dependency Dec 25, 2022
@bluwy bluwy added feat: css p2-nice-to-have Not breaking anything but nice to have (priority) labels Dec 25, 2022
@EastSun5566 EastSun5566 changed the title fix(css): enhance error message for missing preprocessor dependency WIP: fix(css): enhance error message for missing preprocessor dependency Dec 26, 2022
@EastSun5566 EastSun5566 changed the title WIP: fix(css): enhance error message for missing preprocessor dependency fix(css): enhance error message for missing preprocessor dependency Dec 26, 2022
@@ -1504,8 +1504,10 @@ function loadPreprocessor(
return (loadedPreprocessors[lang] = _require(resolved))
} catch (e) {
if (e.code === 'MODULE_NOT_FOUND') {
const packageManager =
process.env.npm_config_user_agent?.split(' ')[0].split('/')[0] || 'npm'
Copy link
Contributor Author

@EastSun5566 EastSun5566 Dec 26, 2022

Choose a reason for hiding this comment

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

I hope that the pkgFromUserAgent function can be reused in the same way as import { pkgFromUserAgent } from '@vite/create-vite' or import { pkgFromUserAgent } from '@vite/shared', but currently, it is not exposed in that way.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's ok to duplicate it for now. It would be nice to move this into utils.ts though, and have a separate function that returns package manager specific commands.

e.g. with yarn and pnpm, we want yarn add and pnpm add instead. That way we can do something like ${installCommand} -D ${lang} instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right... I completely forgot that yarn does not support the i/install command. However, pnpm does support pnpm i <package_name> as an alias for pnpm add <package_name>. Nonetheless, pnpm add is the more officially supported method.

I will refactor it to utils.ts and make it more verbose. Thank you for your suggestion!

@EastSun5566 EastSun5566 marked this pull request as draft December 27, 2022 17:25
@EastSun5566 EastSun5566 changed the title fix(css): enhance error message for missing preprocessor dependency WIP: fix(css): enhance error message for missing preprocessor dependency Dec 27, 2022
@EastSun5566 EastSun5566 changed the title WIP: fix(css): enhance error message for missing preprocessor dependency fix(css): enhance error message for missing preprocessor dependency Jul 28, 2023
@EastSun5566 EastSun5566 marked this pull request as ready for review July 28, 2023 10:50
@EastSun5566
Copy link
Contributor Author

Hi @bluwy

Sorry for the long delay. As previously discussed, I have extracted getPackageManagerCommand to the utils. Could you please take a look at it?

Thank you 🙏

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.

Looks great, thanks!

@patak-dev patak-dev merged commit 65e5c22 into vitejs:main Jul 31, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css 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