fix(ssr): strip NULL_BYTE_PLACEHOLDER before import

[sapphi-red]

Description

This PR revises #6390. That implementation might duplicate moduleGraph entry (not sure if it happens).

Maybe it is safer to go with #9036 for 3.0 and merge this PR in 3.1.

reverts #6390
fixes the issue described at #9036 (it's still worth to merge that PR because it replaces the deprecated url.parse)

Additional context

This line import 'id' becomes import 'resolvedId'.

vite/packages/vite/src/node/ssr/ssrModuleLoader.ts

Lines 87 to 89 in 0cb4b25

	const result =
	mod.ssrTransformResult ||
	(await transformRequest(url, server, { ssr: true }))

Then, that resolvedId is passed to ssrLoadModule here.

vite/packages/vite/src/node/ssr/ssrModuleLoader.ts

Lines 136 to 162 in 0cb4b25

	const ssrImport = async (dep: string) => {
	if (dep[0] !== '.' && dep[0] !== '/') {
	return nodeImport(dep, mod.file!, resolveOptions)
	}
	dep = unwrapId(dep).replace(NULL_BYTE_PLACEHOLDER, '\0')
	if (!isCircular(dep) && !pendingImports.get(dep)?.some(isCircular)) {
	pendingDeps.push(dep)
	if (pendingDeps.length === 1) {
	pendingImports.set(url, pendingDeps)
	}
	const mod = await ssrLoadModule(
	dep,
	server,
	context,
	urlStack,
	fixStacktrace
	)
	if (pendingDeps.length === 1) {
	pendingImports.delete(url)
	} else {
	pendingDeps.splice(pendingDeps.indexOf(dep), 1)
	}
	// return local module to avoid race condition #5470
	return mod
	}
	return moduleGraph.urlToModuleMap.get(dep)?.ssrModule
	}

So for the first instantiateModule call by ssrLoadModule, the raw url is passed to instantiateModule. But for other calls, the resolvedId is passed as url to instantiateModule.

---

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.
  • covered by fix: strip NULL_BYTE_PLACEHOLDER before transform #6390's test

[netlify]

✅ Deploy Preview for vite-docs-main canceled.

Name	Link
🔨 Latest commit	0cb4b25
🔍 Latest deploy log	https://app.netlify.com/sites/vite-docs-main/deploys/62d058be5cbd160009fae161

[patak-dev]

I think this is needed. transformRequest expects an unwrapped+\0 URL (we need a name for the two URL spaces, rollup URL and browser URL?). This may not be related to what #6390 tried to fix, but I think we should always call transformRequest with a rollup URL. Probably not in this PR, but for 3.1 it may be worth allowing transformRequest to accept an URL in any space and always call unwrap+removeImportQuery+use\0 (maybe we could have a toRollupURL() helper that does this? Still unsure about naming).

[patak-dev]

I think the \0 replacement is needed. If we do an unwrapId, we are trying to go from a browser-friendly URL to a valid rollup URL (what plugins expect). I think this is part of not having a helper like toRollupURL that does all the needed transforms.
With this line at the start of ssrLoadModule, it means it can receive both a browser-friendly URL or a Rollup URL and it will normalize it. I think we may want to do this with transformRequest too later, as I said in my prev comment

[patak-dev]

IIUC, this may be the only line needed to do the fix. And it isn't needed because we call ssrLoadModule, but because we check pendingImports and moduleGraph.urlToModuleMap.get(dep), and both should be in the rollup URL space.

[patak-dev]

Here is a previous attempt trying to solve the same issue:

• fix: consistent module graph IDs during transform #8807

I don't think the implementation is right, but it may be useful to check if the test cases are good to merge later.

I think that the PR (with the comments above) should solve the issue of duplicated ids in the module graph due to \0 encoded/decoded mismatches.

We would still need to address your comment about \0 being striped by #9036. parseUrl is used in moduleGraph.resolveUrl to collapse files with and without extension (for .ts for example). This is now being applied to any entry in the module graph but it isn't valid for virtual files. I think that the current handling could also break with files that have ? in them (not a query), but that is something we could review in a minor. I think for this PR, we could skip the check for virtual modules. Something like:

  // for incoming urls, it is important to:
  // 1. remove the HMR timestamp query (?t=xxxx)
  // 2. resolve its extension so that urls with or without extension all map to
  // the same module
  async resolveUrl(url: string, ssr?: boolean): Promise<ResolvedUrl> {
    url = removeImportQuery(removeTimestampQuery(url))
    const resolved = await this.resolveId(url, !!ssr)
    const resolvedId = resolved?.id || url
    if (url !== resolvedId && !url.includes('\0') && !url.startsWith(`virtual:`)) {
      const ext = extname(cleanUrl(resolvedId))
      const { pathname, search, hash } = parseUrl(url)
      if (ext && !pathname!.endsWith(ext)) {
        url = pathname + ext + (search || '') + (hash || '')
      }
    }
    return [url, resolvedId, resolved?.meta]
  }

And then we should also merge #9036.

[patak-dev]

About this comment, and to leave a trace in the main thread of the PR. For 3.1 or maybe 4.0, I think we should clarify the expected URL space for each of the main public functions.

Some definitions:

• Rollup-space URL: the format expected by plugins (unwrapped, no /@fs/, /@id, etc, and has the real \0)
• Browser-space URL: a browser-friendly format because for example \0 is not a valid char (pre-fixed to make it valid and has __x00__)

Right now, we have:

• transformRequest: expects a Rollup-space URL
• ssrLoadModules: it supports both a Rollup-space or Browser-space URL

I proposed in the comments yesterday that we may want to make transformRequest also support both by adding the unwrapping guard at function start.

Supporting both is comfortable, you don't need users to think about what space their URLs are (normally if you are in a middleware you are browser space, and if you are in a plugin you are in rollup space). But also then we can't have proper decode/encode escape hatches (these are implemented right now, but maybe we should?). Users can't have a /@fs folder on their system, because we don't pre-escape them as you would do for special chars like: %%, \\. It could be for example __vite_escaped_@id. Or since all these are at the start, maybe just something like /@fs/@fs. If we want this to be an option, maybe ssrLoadModules shouldn't do any unwrapping.

So maybe we should require ssrLoadModules to be called with a rollup space URL, that makes sense since we are in node there anyways. This will mean that users in plugins and internal requests need to unwrap the imported ids internally. Something that could help here is that in SSR mode, we could avoid wrapping URL in imports. This would make things easier in Node land, but we need to watch out for webworkers so it may not be a good idea.

For reference, the transformRequest only supports rollup based URLs the ssrLoadModules support both from before Vite 2 release 108be94

[sapphi-red]

Thank you for the detailed explanation. Now I think I understand it properly.

I was understanding "URL" stands for browser-space URL and "id" stands for rollup-space URL.
And I thought this PR will fix the issue described at #9036 because rollup-space URL will no longer be passed to ModuleGraph::resolveUrl. But now I realized that my assumptions was incorrect.

I'll create a separate PR to fix the issue described at #9036. (because I noticed that fix will not be related to this one)

[patak-dev]

id is a good name for "Rollup-space URL". Maybe we should review our codebase and only use url when talking about browser space, and always use id when in rollup space. But at this point, url is being used everywhere. This has been confusing to me too.