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(ssr): strip NULL_BYTE_PLACEHOLDER before import #9124

Merged
merged 3 commits into from Jul 18, 2022

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Jul 14, 2022

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'.

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

Then, that resolvedId is passed to ssrLoadModule here.
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

@netlify
Copy link

netlify bot commented Jul 14, 2022

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

url = unwrapId(removeImportQuery(url)).replace(
NULL_BYTE_PLACEHOLDER,
'\0'
)
Copy link
Member

@patak-dev patak-dev Jul 17, 2022

Choose a reason for hiding this comment

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

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).

@@ -38,7 +38,7 @@ export async function ssrLoadModule(
urlStack: string[] = [],
fixStacktrace?: boolean
): Promise<SSRModule> {
url = unwrapId(url).replace(NULL_BYTE_PLACEHOLDER, '\0')
url = unwrapId(url)
Copy link
Member

@patak-dev patak-dev Jul 17, 2022

Choose a reason for hiding this comment

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

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

@@ -137,7 +137,7 @@ async function instantiateModule(
if (dep[0] !== '.' && dep[0] !== '/') {
return nodeImport(dep, mod.file!, resolveOptions)
}
dep = unwrapId(dep)
dep = unwrapId(dep).replace(NULL_BYTE_PLACEHOLDER, '\0')
Copy link
Member

@patak-dev patak-dev Jul 17, 2022

Choose a reason for hiding this comment

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

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
Copy link
Member

patak-dev commented Jul 17, 2022

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

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 patak-dev added the p4-important Violate documented behavior or significantly improves performance (priority) label Jul 17, 2022
@patak-dev
Copy link
Member

patak-dev commented Jul 18, 2022

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 sapphi-red marked this pull request as draft Jul 18, 2022
@sapphi-red
Copy link
Member Author

sapphi-red commented Jul 18, 2022

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
Copy link
Member

patak-dev commented Jul 18, 2022

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.

@patak-dev patak-dev mentioned this pull request Jul 18, 2022
9 tasks
@patak-dev patak-dev marked this pull request as ready for review Jul 18, 2022
@patak-dev patak-dev merged commit c5f2dc7 into vitejs:main Jul 18, 2022
12 checks passed
@sapphi-red sapphi-red deleted the fix/ssr-strip-null-byte-placeholder branch Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants