Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.

@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 requested a review from bluwy December 26, 2022 14:53
@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
# for free to join this conversation on GitHub. Already have an account? # 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