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 error in deepimport #5886

Closed
wants to merge 3 commits into from

Conversation

zhangyuang
Copy link
Contributor

@zhangyuang zhangyuang commented Nov 29, 2021

Description

Mentioned #3039, there are only tryindex when no exports field. It will be error in below code.

Maybe there should tryindex again when no resolved or tryindex always.

image

// error
import Button from 'vant/lib/button'
// succeed
import Button from 'vant/lib/button/index.js'

Additional context

There is a reproduction

$ git clone git@github.com:zhangyuang/vite-ssr-error-require.git
$ git checkout bug/tryindex
$ yarn && yarn dev 

image


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.

@Shinigami92 Shinigami92 added the p3-minor-bug An edge case that only affects very specific usage (priority) label Nov 29, 2021
@gluck
Copy link
Contributor

gluck commented Nov 29, 2021

Hi, the current behavior seems consistent with node resolver:

Welcome to Node.js v17.1.0.
Type ".help" for more information.
> require.resolve('vant/lib/button/index.js')
'/tmp/bar/node_modules/vant/lib/button/index.js'
> require.resolve('vant/lib/button')
Uncaught Error: Cannot find module '/tmp/bar/node_modules/vant/lib/button'
    at createEsmNotFoundErr (node:internal/modules/cjs/loader:954:15)
    at finalizeEsmResolution (node:internal/modules/cjs/loader:947:15)
    at resolveExports (node:internal/modules/cjs/loader:482:14)
    at Function.Module._findPath (node:internal/modules/cjs/loader:522:31)
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:919:27)
    at Function.resolve (node:internal/modules/cjs/helpers:108:19) {
  code: 'MODULE_NOT_FOUND',
  path: '/tmp/bar/node_modules/vant/package.json'
}

The spec may not be clear around whether an export mapping from ./lib/* to ./lib/* should resolve index.js but the implementation doesn't.

Retrying resolve with index lookup seems to defeat exports specs in many (other) ways.

My 2c.

@bluwy
Copy link
Member

bluwy commented Dec 4, 2021

If @gluck confirmed this is intended behaviour by node, I think we should lean on that and not have this change. Unless we figure out the try index only if no exports field comment.

@zhangyuang
Copy link
Contributor Author

Maybe it's vant bug ,let me take a look

@zhangyuang
Copy link
Contributor Author

zhangyuang commented Dec 4, 2021

@bluwy Maybe we can fix the problem in vite layer? The kind of writing is frequent, some third-party ui library like antd, vant which use babel-plugin-import to transform below code

import { Button } from 'antd'

=> import Button from 'antd/lib/button'
import 'antd/lib/button/style'

not tryindex if has exports is not exactly in some case

@bluwy
Copy link
Member

bluwy commented Dec 4, 2021

I don't think Vite should deviate from the spec as it's one of its philosophies. Re-reading try index only if no exports field now, I think it makes sense, since if we have an exports field, that should be respected no matter what.

But regarding vant, it looks like they're not correctly exporting the /lib. I don't see a trailing slash being in the spec, maybe something like:

"./lib/*": "./lib/*/index.js"

would work.

Re antd, they don't seem to be using exports.

@zhangyuang
Copy link
Contributor Author

Ok, thanks a lot 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants