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: use debugger for package resolution warnings #4873

Merged
merged 2 commits into from Sep 8, 2021

Conversation

antfu
Copy link
Member

@antfu antfu commented Sep 7, 2021

Reverts #4822

I think the original behavior (trying to get submodule paths) is expected.

The log creates too much noise that ppl usually don't need to care about. For the debugging purpose stated in #4822, I think it's better to modify local node_modules when debugging.

image

@antfu antfu changed the title Revert "chore: surface package resolution failures" fix: revert #4822 Sep 7, 2021
Copy link
Member

@sodatea sodatea left a comment

Choose a reason for hiding this comment

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

Or, we can use a debug log here?

@benmccann
Copy link
Collaborator

When I've seen this print it's due to a Vite bug: #4425. I think the best solution here would be to fix that bug rather than silently drop the exception. It is causing real issues in my applications, so it's helpful to know it's happening. But if we need to silence it in the meantime I agree that debug would be better

@patak-dev
Copy link
Member

patak-dev commented Sep 8, 2021

+1 to show this with a debug log, there is also a missing break that would make the logs easier to read.

Maybe we could use a warn once with a single short message if not debugging so people know that they may need to dig deeper. "Some packages may have incorrect...." and a pointer to use debug logs to see more

@antfu
Copy link
Member Author

antfu commented Sep 8, 2021

Updated using debugger. Since it scans all the devDependencies, which could include many packages that are not intended to use that way (in the screenshots, @types/* and cli tools for example). I don't think we should throw any message regarding the scanning failures by default here, as it might have many false positives and make users nervous.

A better solution I would think about this is that we record that error silently, and we the import analyzer actually scanned the usages with that package, and then we could warn about that there might be something wrong. For example,

{
  "devDependencies": {
     // here we assume both packages have something wrong with their exports
	 "@types/node": "*",
     "lodash": "*"
  }
}
// some-module.js
import set from 'lodash/set'

Warning when scanned some-module.js:

Trying to importing lodash/set in some-module.js but lodash does not export the module or there is something wrong during the package resolving, externalization skipped. \n {{error}}

@patak-dev patak-dev changed the title fix: revert #4822 fix: use debugger for package resolution warnings Sep 8, 2021
@patak-dev patak-dev merged commit 38de2c9 into main Sep 8, 2021
@patak-dev patak-dev deleted the revert-4822-surface-resolution-failures branch September 8, 2021 12:32
aleclarson pushed a commit to aleclarson/vite that referenced this pull request Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants