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.

Sign up for GitHub

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

fix: prevent null pathname error #9188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1236a99
fix: prevent null pathname error
sapphi-red Jul 18, 2022
2d08edc
refactor: replace url.parse with new URL
sapphi-red Jul 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions packages/vite/src/node/server/moduleGraph.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { extname } from 'node:path'
import { parse as parseUrl } from 'node:url'
import type { ModuleInfo, PartialResolvedId } from 'rollup'
import { isDirectCSSRequest } from '../plugins/css'
import {
Expand Down Expand Up @@ -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 •
edited
Loading

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!

const ext = extname(cleanUrl(resolvedId))
const { pathname, search, hash } = parseUrl(url)
if (ext && !pathname!.endsWith(ext)) {
url = pathname + ext + (search || '') + (hash || '')
if (
url !== resolvedId &&
!url.includes('\0') &&
!url.startsWith(`virtual:`)
) {
const ext = extname(cleanUrl(resolvedId))
const { pathname, search, hash } = new URL(url, 'relative://')
if (ext && !pathname!.endsWith(ext)) {
url = pathname + ext + search + hash
}
}
return [url, resolvedId, resolved?.meta]
}
Expand Down
6 changes: 6 additions & 0 deletions playground/resolve/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ <h2>Monorepo linked dep</h2>
<h2>Plugin resolved virtual file</h2>
<p class="virtual"></p>

<h2>Plugin resolved virtual file (#9036)</h2>
<p class="virtual-9036"></p>

<h2>Plugin resolved custom virtual file</h2>
<p class="custom-virtual"></p>

Expand Down Expand Up @@ -221,6 +224,9 @@ <h2>resolve package that contains # in path</h2>
import { msg as virtualMsg } from '@virtual-file'
text('.virtual', virtualMsg)

import { msg as virtualMsg9036 } from 'virtual:file-9036.js'
text('.virtual-9036', virtualMsg9036)

import { msg as customVirtualMsg } from '@custom-virtual-file'
text('.custom-virtual', customVirtualMsg)

Expand Down
16 changes: 16 additions & 0 deletions playground/resolve/vite.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ const { normalizePath } = require('vite')
const virtualFile = '@virtual-file'
const virtualId = '\0' + virtualFile

const virtualFile9036 = 'virtual:file-9036.js'
const virtualId9036 = '\0' + virtualFile9036

const customVirtualFile = '@custom-virtual-file'
const { a } = require('./config-dep')

Expand Down Expand Up @@ -44,6 +47,19 @@ module.exports = {
}
}
},
{
name: 'virtual-module-9036',
resolveId(id) {
if (id === virtualFile9036) {
return virtualId9036
}
},
load(id) {
if (id === virtualId9036) {
return `export const msg = "[success] from virtual file #9036"`
}
}
},
{
name: 'custom-resolve',
resolveId(id) {
Expand Down