Skip to content

Conversation

@natemoo-re
Copy link
Contributor

@natemoo-re natemoo-re commented Dec 15, 2021

Description

Adds fallback to viteResolve logic to just return absolute ids if tryNodeResolve doesn't return a match.

Astro's test suite is down to a single failure with this change.

Additional context

Astro is attempting to upgrade to [email protected] but we've been hitting failures in CI. Because Vite is hooking require for SSR, all resolution is running through Vite for these tests.

This PR modifies still runs tryNodeResolve, but if a Vite-specific match is not found and the path is already absolute, we just fallback to the absolute path.


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.

importer: string,
options = resolveOptions
) => {
const resolved = tryNodeResolve(id, importer, options, false)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that tryNodeResolve runs first, but won't return a match.

The problematic id is already a fully resolved, absolute path to a file that does exist... maybe this needs to be fixed inside of tryNodeResolve?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe this needs to be fixed inside of tryNodeResolve?

Vite's internal resolve plugin checks for absolute paths before tryNodeResolve. See here:

// absolute fs paths
if (path.isAbsolute(id) && (res = tryFsResolve(id, options))) {
isDebug && debug(`[fs] ${colors.cyan(id)} -> ${colors.dim(res)}`)
return res
}

Comment on lines 218 to +221
if (!resolved) {
if (path.isAbsolute(id)) {
return id
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Attempt: if tryNodeResolve returns undefined but our id is an absolute path, it might be fully resolved already. Just return it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason we shouldn't check for absolute path before the tryNodeResolve call?

@btakita
Copy link

btakita commented Dec 25, 2021

Does #5495 also fix the issue with absolute paths?

@Niputi Niputi requested a review from aleclarson December 28, 2021 23:47
@bluwy
Copy link
Member

bluwy commented Jan 20, 2022

@natemoo-re It looks like #6488 is solving the same issue as this. Is this still needed?

(I forgot about this PR when making my fix there 😅)

@aleclarson aleclarson closed this Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants