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: prevent null pathname error #9188

Merged
merged 2 commits into from
Jul 18, 2022
Merged

Conversation

sapphi-red
Copy link
Member

Description

This PR fixes the issue described at #9036.

The issue happens when

  • the resolved id starts with \0virtual:
  • the resolved id ends with an extension

close #9036

Additional context


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.

sapphi-red and others added 2 commits July 18, 2022 21:53
Co-Authored-By: Christoph Werner <codepunkt@users.noreply.github.com>
@sapphi-red sapphi-red added the p4-important Violate documented behavior or significantly improves performance (priority) label Jul 18, 2022
@@ -237,10 +236,16 @@ export class ModuleGraph {
url = removeImportQuery(removeTimestampQuery(url))
const resolved = await this.resolveId(url, !!ssr)
const resolvedId = resolved?.id || url
Copy link
Member Author

@sapphi-red sapphi-red Jul 18, 2022

Choose a reason for hiding this comment

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

resolveUrl is called with both rollup-space URL and browser-space URL.
Is this function intended to be called with both? Does this need to be resolved to a same url? (currently it doesn't)

You can check this by adding this line here and running playground/resolve.

if (url.includes('@virtual-file')) console.trace(url, resolved?.id, resolved?.id.replace('\0', '!0!'))

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 isn't clear, it is a similar context to ssrLoadModule as explained in #9124 (comment). In Vite 4 we could review if we should for resolveUrl to always be in a given space, but since ssrLoadModule already accepts both, maybe we should also unwrap the id and replace to \0 as a guard at the start of it? If not we should review where is it being called with a URL out in browser space and pre-unwrap it.
Looks like this ensure has receiving browser-space URLs since before the release of Vite 2. See 8d8e2cc#diff-f2f744fef86a2c562dd5142240912f7a2d28404fac536740a2424daf628aa609R208

I don't know what is the best here during v3. The module graph is user-facing, so if we start accepting both then it may be hard to properly decode/encode.

Maybe we could still try to keep this function as is, and pre-unwrap in the places we are missing? We could already create the toRollupURL helper and use it for these places?

For reference, in the module graph we have:

  • urlToModuleMap that maps a Rollup-space URL to a Module
  • idToModuleMap that maps a resolved id to a Module

Since we are calling resolveUrl with both internally, we could review if what we really want in the graph is Browser-space URL or Rollup-space URLs, but since in both transformRequest and in ssrLoadModule we are using Rollup-space URLs when calling it I think it the above is fine and we should expect to see \0 and unwrapped ids (no /@fs/, etc) when exploring the module graph by URL. cc @antfu, just in case you have seen this in your plugins.

Copy link
Member Author

Choose a reason for hiding this comment

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

Umm, it seems it requires much more work. I feel I need to read though all the code if I'm going to tackle this.
I'll leave this one as-is in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, and maybe it is better, as it is unrelated to the fix this PR is targeting.
We can continue to work on this on future smaller PRs, and do a full review for 3.1.
Thanks a lot @sapphi-red!

@patak-dev patak-dev merged commit d66ffd0 into vitejs:main Jul 18, 2022
@sapphi-red sapphi-red deleted the fix/null-pathname branch July 18, 2022 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants