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

Jump to bottom

fix(ssr): strip NULL_BYTE_PLACEHOLDER before import #9124

Merged
merged 3 commits into from Jul 18, 2022
Merged

fix(ssr): strip NULL_BYTE_PLACEHOLDER before import #9124

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0cb4b25
fix(ssr): strip NULL_BYTE_PLACEHOLDER before import
sapphi-red Jul 14, 2022
2d91f25
Revert "fix(ssr): strip NULL_BYTE_PLACEHOLDER before import"
sapphi-red Jul 18, 2022
22556b1
fix(ssr): convert to rollup URL in instantiateModule
sapphi-red Jul 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 1 addition & 7 deletions packages/vite/src/node/plugins/importAnalysis.ts
View file
Open in desktop
Expand Up @@ -38,11 +38,9 @@ import {
moduleListContains,
normalizePath,
prettifyUrl,
removeImportQuery,
stripBomTag,
timeFrom,
transformStableResult,
unwrapId
transformStableResult
} from '../utils'
import type { ResolvedConfig } from '../config'
import type { Plugin } from '../plugin'
Expand Down Expand Up @@ -706,10 +704,6 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
// by the deps optimizer
if (config.server.preTransformRequests && staticImportedUrls.size) {
staticImportedUrls.forEach(({ url, id }) => {
url = unwrapId(removeImportQuery(url)).replace(
NULL_BYTE_PLACEHOLDER,
'\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 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).

transformRequest(url, server, { ssr }).catch((e) => {
if (e?.code === ERR_OUTDATED_OPTIMIZED_DEP) {
// This are expected errors
Expand Down
4 changes: 2 additions & 2 deletions packages/vite/src/node/ssr/ssrModuleLoader.ts
View file
Open in desktop
Expand Up @@ -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

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


// when we instantiate multiple dependency modules in parallel, they may
// point to shared modules. We need to avoid duplicate instantiation attempts
Expand Down Expand Up @@ -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

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.

if (!isCircular(dep) && !pendingImports.get(dep)?.some(isCircular)) {
pendingDeps.push(dep)
if (pendingDeps.length === 1) {
Expand Down