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(resolve): use DEFAULT_EXTENSIONS when resolving file from node_modules #11473

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

zscumt123
Copy link

@zscumt123 zscumt123 commented Dec 23, 2022

fix #11467

Description

in this case

//vite.config.ts
import { defineConfig } from "vite";
export default defineConfig({
  resolve: {
    extensions: ['.ts']
  }
})

some dependence from node_modules, package.json without file extension

{
  "main": "./lib/form_data",
  "browser": "./lib/browser",
}

this can cause dependency resolved fail


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 added p2-nice-to-have Not breaking anything but nice to have (priority) and removed breaking change labels Dec 28, 2022
@patak-dev
Copy link
Member

I don't know if this is the best thing with the current DEFAULT_EXTENSIONS, because users aren't able to avoid checking for ts or jsx in node_modules after this PR and could affect perf in some projects.

If we move forward, maybe we could do it in Vite 5 and have something like DEFAULT_NODE_MODULES_EXTENSIONS (no .ts, .jsx, etc) and a new option resolve.nodeModulesExtensions?

@bluwy bluwy marked this pull request as draft March 30, 2023 07:03
@bluwy bluwy changed the title fix: should use DEFAULT_EXTENSIONS when analysis file from node_modules(#11467) fix(resolve): use DEFAULT_EXTENSIONS when resolving file from node_modules Mar 30, 2023
@bluwy
Copy link
Member

bluwy commented Mar 30, 2023

In the last meeting, we discussed where this could be good for users to opt-in to stricter extensions in their source code, while still having node_modules work.

Rebased this, and I realized the previous implementation isn't exactly correct, because some createResolver can define their own set of extensions to resolve, e.g. CSS extensions only. We only want to fallback to DEFAULT_EXTENSIONS in node_modules if we're using the same extensions specified in resolve.extensions user config, which I've updated to do so.

I'm not very confident with the new changes, I think this needs some more tests. Marking as draft for now.

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
Archived in project
Development

Successfully merging this pull request may close these issues.

Failed to resolve entry for package "form-data"
3 participants