diff --git a/.changeset/thirty-birds-build.md b/.changeset/thirty-birds-build.md new file mode 100644 index 000000000..3bf269368 --- /dev/null +++ b/.changeset/thirty-birds-build.md @@ -0,0 +1,30 @@ +--- +'@cloudflare/next-on-pages': patch +--- + +fix: implement route specific global scoping strategy + +currently routes all share the same global scope, this can be problematic and cause +race conditions and failures + +One example of this is the following code that is present in route function files: + +```ts +self.webpackChunk_N_E = ... +``` + +and + +```ts +self.webpackChunk_N_E.push(...) +``` + +this indicates that an in-memory global collection of the webpack chunks is shared by all routes, +this combined with the fact that chunks can have their own module state this can easily cause routes to conflict with each other at runtime. + +So, in order to solve the above issue, all route functions are wrapped in a function which accepts as parameters, thus overrides, the `self`, `globalThis` and `global` symbols. The symbols +will be resolved with proxies that redirect setters to route-scoped in-memory maps and +getters to the above mentioned map's values and fallback to the original symbol values otherwise +(i.e. `globalThis` will be overridden by a proxy that, when setting values, sets them in a separate +location and, when getting values, gets them from said location if present there or from the real +`globalThis` otherwise) diff --git a/packages/next-on-pages/src/buildApplication/processVercelFunctions/dedupeEdgeFunctions.ts b/packages/next-on-pages/src/buildApplication/processVercelFunctions/dedupeEdgeFunctions.ts index f3295237b..5a9721c6c 100644 --- a/packages/next-on-pages/src/buildApplication/processVercelFunctions/dedupeEdgeFunctions.ts +++ b/packages/next-on-pages/src/buildApplication/processVercelFunctions/dedupeEdgeFunctions.ts @@ -101,13 +101,20 @@ async function processFunctionIdentifiers( if (importPath) { // Dedupe and update collected imports. - const { updatedContents } = await processImportIdentifier( - { type, identifier, start, end, importPath, info: identifierInfo }, - { fileContents, entrypoint, newFnLocation, fnConfig: fnInfo.config }, - opts, - ); + const { updatedContents, newImportToPrepend } = + await processImportIdentifier( + { type, identifier, start, end, importPath, info: identifierInfo }, + { + fileContents, + entrypoint, + newFnLocation, + fnConfig: fnInfo.config, + }, + opts, + ); fileContents = updatedContents; + importsToPrepend.push(newImportToPrepend); } else if (identifierInfo.consumers.length > 1) { // Only dedupe code blocks if there are multiple consumers. const { updatedContents, newFilePath, newImport, wasmImports } = @@ -143,9 +150,10 @@ async function processFunctionIdentifiers( // Build the identifier files before building the function's file. await Promise.all( - [...identifierPathsToBuild].map(async path => - buildFile(await readFile(path, 'utf8'), path), - ), + [...identifierPathsToBuild].map(async path => { + const fileContents = await functionifyFileContent(path); + return buildFile(fileContents, path); + }), ); // If wasm identifier is used in code block, prepend the import to the code block's file. @@ -168,6 +176,27 @@ async function processFunctionIdentifiers( await Promise.all(functionBuildPromises); } +/** + * Given a standard ESM file (without imports) it converts it to a function call that returns + * an object with the various exports set as its fields + * + * The function allows us to override global symbols such as `self`, `globalThis` and `global` + * (which are used as the function's parameter names) + * + * @param path the path of the ESM file + * @returns the converted file content + */ +async function functionifyFileContent(path: string) { + const originalFileContents = await readFile(path, 'utf8'); + return ` + ${ + /* Note: we need to make sure that the named exports object is defined since that is used inside the file */ '' + } + const ${namedExportsObjectName} = {}; + export const ${getNamedExportsFunctionName} = ((self, globalThis, global) => { ${originalFileContents} return ${namedExportsObjectName}; }); + `; +} + /** * Builds a new file for an Edge function. * @@ -184,7 +213,7 @@ async function buildFunctionFile( { importsToPrepend }: { importsToPrepend: NewImportInfo[] }, { workerJsDir, nopDistDir }: ProcessVercelFunctionsOpts, ): Promise<{ buildPromise: Promise }> { - let functionImports = ''; + const functionImports: string[] = []; // Group the identifier imports by the keys for each path. const groupedImports = importsToPrepend.reduce((acc, { key, path }) => { @@ -193,21 +222,43 @@ async function buildFunctionFile( return acc; }, new Map()); - groupedImports.forEach((keys, path) => { - const relativeImportPath = getRelativePathToAncestor({ - from: newFnLocation, - relativeTo: nopDistDir, - }); + let chunkMapIdx = 0; + const chunksExportsMap = new Map>(); + + const relativeImportPath = getRelativePathToAncestor({ + from: newFnLocation, + relativeTo: nopDistDir, + }); + + groupedImports.forEach((exports, path) => { const importPath = normalizePath( join(relativeImportPath, addLeadingSlash(path)), ); - functionImports += `import { ${keys} } from '${importPath}';\n`; + if (path.endsWith('.wasm')) { + // if we're dealing with a wasm file there is a single default export to deal with + const defaultExport = exports; + // we don't need/want to apply any code transformation for wasm imports + functionImports.push(`import ${defaultExport} from '${path}';`); + return; + } + + const getNamedExportsFunctionWithId = `${getNamedExportsFunctionName}_${chunkMapIdx++}`; + const exportKeys = exports.split(','); + chunksExportsMap.set(getNamedExportsFunctionWithId, new Set(exportKeys)); + functionImports.push( + `import { ${getNamedExportsFunctionName} as ${getNamedExportsFunctionWithId} } from '${importPath}';`, + ); }); fnInfo.outputPath = relative(workerJsDir, newFnPath); - const finalFileContents = `${functionImports}${fileContents}`; + const finalFileContents = iifefyFunctionFile( + fileContents, + functionImports, + fnInfo, + chunksExportsMap, + ); const buildPromise = buildFile(finalFileContents, newFnPath, { relativeTo: nopDistDir, }).then(async () => { @@ -225,6 +276,60 @@ type BuildFunctionFileOpts = { newFnPath: string; }; +/** + * Given the content of a function file it converts/wraps it into an iife that overrides the function's contents with an iffe call that + * overrides global symbols with route-specific proxies (for more details see: templates/_worker.js/routesIsolation.ts) + * + * @param fileContents the function file's contents + * @param functionImports the imports that need to be added to the file + * @param functionInfo the function's information + * @param chunksExportsMap a map containing getters and chunks identifiers being used by the function + * @returns the updated/iifefied file content + */ +function iifefyFunctionFile( + fileContents: string, + functionImports: string[], + functionInfo: FunctionInfo, + chunksExportsMap: Map>, +): string { + const wrappedContent = ` + export default ((self, globalThis, global) => { + ${fileContents + // it looks like there can be direct references to _ENTRIES (i.e. `_ENTRIES` instead of `globalThis._ENTRIES` etc...) + // we have to update all such references otherwise our proxying won't take effect on those + // (note: older versions of the Vercel CLI (v31 and older) used to declare _ENTRIES as "let _ENTRIES = {};", so we do + // need to make sure that we don't add `globalThis.` in these cases (if we were to drop support for those older versions + // the below line to: ".replace(/([^.])_ENTRIES/g, '$1globalThis._ENTRIES')") + .replace(/(? { + const namedExportsObjectWithId = `__exportsOf${getNamedExportsFunctionWithId}`; + return [ + `const ${namedExportsObjectWithId} = ${getNamedExportsFunctionWithId}(proxy, proxy, proxy);`, + ...[...keys.keys()].map( + key => `const ${key} = ${namedExportsObjectWithId}["${key}"];`, + ), + ]; + }, + ); + + return [ + ...functionImports, + proxyCall, + ...chunksExtraction, + wrappedContent, + ].join('\n'); +} + /** * Prepends Wasm imports to a code block's built file. * @@ -246,7 +351,7 @@ async function prependWasmImportsToCodeBlocks( relativeTo: nopDistDir, }); - let functionImports = ''; + const functionImports: string[] = []; for (const identifier of wasmImports) { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion @@ -254,11 +359,14 @@ async function prependWasmImportsToCodeBlocks( const wasmImportPath = normalizePath( join(relativeImportPath, newDest as string), ); - functionImports += `import ${identifier} from "${wasmImportPath}";\n`; + functionImports.push(`import ${identifier} from "${wasmImportPath}"`); } const oldContents = await readFile(filePath); - await writeFile(filePath, `${functionImports}${oldContents}`); + await writeFile( + filePath, + `${functionImports.join(';')};${oldContents}`, + ); }, ), ); @@ -268,12 +376,13 @@ async function prependWasmImportsToCodeBlocks( * Processes an import path identifier. * * - Moves the imported file to the new location if it doesn't exist. - * - Updates the file contents to import from the new location. + * - Updates the file contents to remove the import. + * - Returns the information for the new import. * * @param ident The import path identifier to process. * @param processOpts Contents of the function's file, the function's entrypoint, and the new path. * @param opts Options for processing the function. - * @returns The updated file contents. + * @returns The updated file contents alongside the new import information. */ async function processImportIdentifier( ident: RawIdentifierWithImport & { info: IdentifierInfo }, @@ -284,7 +393,7 @@ async function processImportIdentifier( fnConfig, }: ProcessImportIdentifierOpts, { nopDistDir, workerJsDir }: ProcessVercelFunctionsOpts, -): Promise<{ updatedContents: string }> { +): Promise<{ updatedContents: string; newImportToPrepend: NewImportInfo }> { const { type, identifier, start, end, importPath, info } = ident; let updatedContents = fileContents; @@ -320,14 +429,20 @@ async function processImportIdentifier( }); const newImportPath = normalizePath(join(relativeImportPath, info.newDest)); - const newVal = `import ${identifier} from "${newImportPath}";`; + // let's remove the original import since it will be re-added later when appropriate updatedContents = replaceLastSubstringInstance( updatedContents, codeBlock, - newVal, + '', ); - return { updatedContents }; + return { + updatedContents, + newImportToPrepend: { + key: identifier, + path: newImportPath, + }, + }; } type ProcessImportIdentifierOpts = { @@ -383,7 +498,7 @@ async function processCodeBlockIdentifier( .forEach(key => wasmImports.push(key)); const buffer = Buffer.from( - `export const ${identifierKey} = ${codeBlock}\n`, + `${namedExportsObjectName}["${identifierKey}"] = ${codeBlock}\n`, 'utf8', ); @@ -554,3 +669,43 @@ async function processBundledAssets( } } } + +/** + * When performing the various code tweaking we never introduce standard named ESM exports, since those would + * be invalid anyways since each js file content gets wrapped into a function anyways. + * + * Note: route function files don't have named exports to this is only needed for the other files such + * as the manifest ones and the webpack chunks ones + * + * Instead of standard named exports we simply set named exports onto an object which gets then returned by the + * file wrapper function. + * + * + * Example: + * when introducing a new export, instead of introducing: + * ``` + * export const a = ... + * ``` + * we introduce something like: + * ``` + * NAMED_EXPORTS_OBJECT["a"] = ... + * ``` + * and make sure that such object is always declared and returned by the function wrapping the file's contents. + * + * This is the name of the object used for such exports. + */ +const namedExportsObjectName = '__namedExportsObject'; + +/** + * The strategy of exporting exports via an returned object relies on the fact that there is a function that returns such object. + * + * Example: + * ``` + * import { __getNamedExports } from '...'; + * const obj = __getNamedExports(...); + * // obj contains the various exports as standard object properties + * ``` + * + * This is the name of such function. + */ +const getNamedExportsFunctionName = '__getNamedExports'; diff --git a/packages/next-on-pages/templates/_worker.js/index.ts b/packages/next-on-pages/templates/_worker.js/index.ts index cf29e2d39..50cf212af 100644 --- a/packages/next-on-pages/templates/_worker.js/index.ts +++ b/packages/next-on-pages/templates/_worker.js/index.ts @@ -1,5 +1,6 @@ import { SUSPENSE_CACHE_URL } from '../cache'; import { handleRequest } from './handleRequest'; +import { setupRoutesIsolation } from './routesIsolation'; import { adjustRequestForVercel, handleImageResizingRequest, @@ -22,6 +23,7 @@ declare const __ALSes_PROMISE__: Promise(); + + return new Proxy(globalThis, { + get: (_, property) => { + if (overrides.has(property)) { + return overrides.get(property); + } + return Reflect.get(globalThis, property); + }, + set: (_, property, value) => { + if (sharedGlobalProperties.has(property)) { + // this property should be shared across all routes + return Reflect.set(globalThis, property, value); + } + overrides.set(property, value); + return true; + }, + }); +} + +/** + * There are some properties that do need to be shared across all different routes, so we collect + * them in this set and skip the global scope proxying for them + */ +const sharedGlobalProperties = new Set([ + '_nextOriginalFetch', + 'fetch', + '__incrementalCache', +]); + +type RoutesIsolation = { + _map: Map; + getProxyFor: (route: string) => unknown; +}; + +declare global { + // eslint-disable-next-line no-var + var __nextOnPagesRoutesIsolation: RoutesIsolation; +} diff --git a/pages-e2e/features/_utils/getAssertVisible.ts b/pages-e2e/features/_utils/getAssertVisible.ts index a7f7fbe35..2119f6f86 100644 --- a/pages-e2e/features/_utils/getAssertVisible.ts +++ b/pages-e2e/features/_utils/getAssertVisible.ts @@ -19,7 +19,6 @@ async function assertVisible( page: Page, ...[selector, options]: Parameters ): Promise { - let isVisible = false; for (const _attempt of [0, 1, 2, 3, 4, 5]) { const locator = page.locator(selector, options); try { diff --git a/pages-e2e/features/customEntrypoint/assets/custom-entrypoint.ts b/pages-e2e/features/customEntrypoint/assets/custom-entrypoint.ts index 991dfa56a..3884220a9 100644 --- a/pages-e2e/features/customEntrypoint/assets/custom-entrypoint.ts +++ b/pages-e2e/features/customEntrypoint/assets/custom-entrypoint.ts @@ -1,3 +1,4 @@ +//@ts-nocheck import nextOnPagesHandler from '@cloudflare/next-on-pages/fetch-handler'; export default { diff --git a/pages-e2e/fixtures/app14.0.0/main.fixture b/pages-e2e/fixtures/app14.0.0/main.fixture index 4b0cc891d..cdf6a29e9 100644 --- a/pages-e2e/fixtures/app14.0.0/main.fixture +++ b/pages-e2e/fixtures/app14.0.0/main.fixture @@ -4,7 +4,6 @@ "appRouting", "appConfigsTrailingSlashTrue", "appWasm", - "appServerActions", "appGetRequestContext" ], "localSetup": "npm install",