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: ignore invalid i18n functions with valid alternatives #466

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/rich-squids-visit.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@cloudflare/next-on-pages': patch
---

Ignore invalid nodejs i18n functions with valid alternatives.
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ async function prepareAndBuildWorker(
workerJsDir,
nopDistDir: join(workerJsDir, '__next-on-pages-dist__'),
disableChunksDedup,
vercelConfig,
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export type CollectedFunctions = {
edgeFunctions: Map<string, FunctionInfo>;
prerenderedFunctions: Map<string, FunctionInfo>;
invalidFunctions: Map<string, FunctionInfo>;
ignoredFunctions: Map<string, FunctionInfo>;
ignoredFunctions: Map<string, FunctionInfo & { reason?: string }>;
dario-piotrowicz marked this conversation as resolved.
Show resolved Hide resolved
};

export type FunctionInfo = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,10 @@ export async function processEdgeFunctions(
break;
}
case 'ignore': {
ignoredFunctions.set(path, fnInfo);
ignoredFunctions.set(path, {
reason: entrypointStatus.reason,
...fnInfo,
});
edgeFunctions.delete(path);
break;
}
Expand Down Expand Up @@ -89,7 +92,9 @@ async function checkEntrypoint(
relativePath: string,
entrypoint: string,
): Promise<
{ value: 'ignore' | 'invalid' } | { value: 'valid'; finalEntrypoint: string }
| { value: 'invalid' }
| { value: 'ignore'; reason: string }
| { value: 'valid'; finalEntrypoint: string }
> {
let finalEntrypoint = entrypoint;

Expand All @@ -109,7 +114,7 @@ async function checkEntrypoint(
cliWarn(
`Detected an invalid middleware function for ${relativePath}. Skipping...`,
);
return { value: 'ignore' };
return { value: 'ignore', reason: 'invalid middleware function' };
}

return { value: 'invalid' };
Expand Down Expand Up @@ -196,7 +201,10 @@ function replaceRscWithNonRsc(
tempFunctionsMap.delete(formattedPath);
edgeFunctions.delete(path);
invalidFunctions.delete(path);
ignoredFunctions.set(path, rscFnInfo);
ignoredFunctions.set(path, {
reason: 'unnecessary rsc function',
...rscFnInfo,
});
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export async function processVercelFunctions(

await processEdgeFunctions(collectedFunctions);

await checkInvalidFunctions(collectedFunctions);
await checkInvalidFunctions(collectedFunctions, opts);

const identifiers = await dedupeEdgeFunctions(collectedFunctions, opts);

Expand All @@ -36,6 +36,7 @@ export type ProcessVercelFunctionsOpts = {
workerJsDir: string;
nopDistDir: string;
disableChunksDedup?: boolean;
vercelConfig: VercelConfig;
};

export type ProcessedVercelFunctions = {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { gtr as versionGreaterThan, coerce } from 'semver';
import { cliError, cliWarn } from '../../cli';
import { getPackageVersion } from '../packageManagerUtils';
import { stripFuncExtension } from '../../utils';
import { formatRoutePath, stripFuncExtension } from '../../utils';
import type { CollectedFunctions, FunctionInfo } from './configs';
import { join, resolve } from 'path';
import type { ProcessVercelFunctionsOpts } from '.';

/**
* Checks if there are any invalid functions from the Vercel build output.
Expand All @@ -14,12 +15,16 @@ import { join, resolve } from 'path';
* If however the build output can't be used, an error message will be printed and the process will exit.
*
* @param collectedFunctions Collected functions from the Vercel build output.
* @param opts Options for processing Vercel functions.
*/
export async function checkInvalidFunctions(
collectedFunctions: CollectedFunctions,
opts: Pick<ProcessVercelFunctionsOpts, 'functionsDir' | 'vercelConfig'>,
): Promise<void> {
await tryToFixNotFoundRoute(collectedFunctions);

await tryToFixI18nFunctions(collectedFunctions, opts);

if (collectedFunctions.invalidFunctions.size > 0) {
await printInvalidFunctionsErrorMessage(
collectedFunctions.invalidFunctions,
Expand Down Expand Up @@ -71,6 +76,71 @@ async function tryToFixNotFoundRoute(
}
}

/**
* Tries to fix potential unnecessary and invalid i18n functions from the Vercel build output.
*
* This is a workaround for Vercel creating invalid Node.js i18n functions in the build output, and
* is achieved by combing through the Vercel build output config to find i18n keys that match the
* invalid functions.
*
* @param collectedFunctions Collected functions from the Vercel build output.
* @param opts Options for processing Vercel functions.
*/
async function tryToFixI18nFunctions(
{ edgeFunctions, invalidFunctions, ignoredFunctions }: CollectedFunctions,
{
vercelConfig,
functionsDir,
}: Pick<ProcessVercelFunctionsOpts, 'functionsDir' | 'vercelConfig'>,
): Promise<void> {
if (!invalidFunctions.size || !vercelConfig.routes?.length) {
return;
}

const foundI18nKeys = vercelConfig.routes.reduce((acc, route) => {
if ('handle' in route) return acc;

// Matches the format used in certain source route entries in the build output config.
// e.g. "src": "/(?<nextLocale>default|en|ja)(/.*|$)"
/\(\?<nextLocale>([^)]+)\)/
.exec(route.src)?.[1]
?.split('|')
?.forEach(locale => acc.add(locale));

return acc;
}, new Set<string>());

if (!foundI18nKeys.size) {
// no i18n keys found in the build output config, so we can't fix anything
return;
}

for (const [fullPath, fnInfo] of invalidFunctions.entries()) {
for (const i18nKey of foundI18nKeys) {
const firstRouteSegment = stripFuncExtension(fnInfo.relativePath)
.replace(/^\//, '')
.split('/')[0];

if (firstRouteSegment === i18nKey) {
const pathWithoutI18nKey = fnInfo.relativePath
.replace(new RegExp(`^/${i18nKey}.func`), '/index.func')
.replace(new RegExp(`^/${i18nKey}/`), '/');
const fullPathWithoutI18nKey = join(functionsDir, pathWithoutI18nKey);
james-elicx marked this conversation as resolved.
Show resolved Hide resolved

const edgeFn = edgeFunctions.get(fullPathWithoutI18nKey);
if (edgeFn) {
invalidFunctions.delete(fullPath);
ignoredFunctions.set(fullPath, {
reason: 'unnecessary invalid i18n function',
...fnInfo,
});
edgeFn.route?.overrides?.push(formatRoutePath(fnInfo.relativePath));
}
}
}
}
}

/**
* Prints an error message for the invalid functions from the Vercel build output.
*
Expand Down
6 changes: 3 additions & 3 deletions packages/next-on-pages/tests/_helpers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,7 @@ export async function createRouterTestData(
workerJsDir: workerJsDir,
nopDistDir: join(workerJsDir, '__next-on-pages-dist__'),
disableChunksDedup: true,
vercelConfig: { version: 3 },
});

const staticAssets = await getVercelStaticAssets();
Expand Down Expand Up @@ -402,9 +403,8 @@ export async function collectFunctionsFrom(
});

await processOutputDir(outputDir, await getVercelStaticAssets());
const collectedFunctions = await collectFunctionConfigsRecursively(
functionsDir,
);
const collectedFunctions =
await collectFunctionConfigsRecursively(functionsDir);
james-elicx marked this conversation as resolved.
Show resolved Hide resolved

return { collectedFunctions, restoreFsMock: () => mockFs.restore() };
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
import { describe, test, expect, afterEach, vi } from 'vitest';
import mockFs from 'mock-fs';
import {
collectFunctionsFrom,
mockConsole,
edgeFuncDir,
nodejsFuncDir,
getRouteInfo,
} from '../../../_helpers';
import { resolve } from 'path';
import { processEdgeFunctions } from '../../../../src/buildApplication/processVercelFunctions/edgeFunctions';
import { checkInvalidFunctions } from '../../../../src/buildApplication/processVercelFunctions/invalidFunctions';

const functionsDir = resolve('.vercel/output/functions');

describe('checkInvalidFunctions', () => {
afterEach(() => mockFs.restore());

test('should ignore i18n index with valid alternative', async () => {
james-elicx marked this conversation as resolved.
Show resolved Hide resolved
const { collectedFunctions, restoreFsMock } = await collectFunctionsFrom({
functions: {
'index.func': edgeFuncDir,
'en.func': nodejsFuncDir,
},
});

await processEdgeFunctions(collectedFunctions);
await checkInvalidFunctions(collectedFunctions, {
functionsDir,
vercelConfig: {
version: 3,
routes: [{ src: '/(?<nextLocale>fr|en|nl)(/.*|$)' }],
},
});
restoreFsMock();

const { edgeFunctions, invalidFunctions, ignoredFunctions } =
collectedFunctions;

expect(edgeFunctions.size).toEqual(1);
expect(getRouteInfo(edgeFunctions, 'index.func')).toEqual({
path: '/index',
overrides: ['/', '/en'],
});

expect(invalidFunctions.size).toEqual(0);

expect(ignoredFunctions.size).toEqual(1);
expect(ignoredFunctions.has(resolve(functionsDir, 'en.func'))).toEqual(
true,
);
});

test('should ignore i18n nested route with valid alternative', async () => {
const { collectedFunctions, restoreFsMock } = await collectFunctionsFrom({
functions: {
test: { 'route.func': edgeFuncDir },
en: { test: { 'route.func': nodejsFuncDir } },
},
});

await processEdgeFunctions(collectedFunctions);
await checkInvalidFunctions(collectedFunctions, {
functionsDir,
vercelConfig: {
version: 3,
routes: [{ src: '/(?<nextLocale>fr|en|nl)(/.*|$)' }],
},
});
restoreFsMock();

const { edgeFunctions, invalidFunctions, ignoredFunctions } =
collectedFunctions;

expect(edgeFunctions.size).toEqual(1);
expect(getRouteInfo(edgeFunctions, 'test/route.func')).toEqual({
path: '/test/route',
overrides: ['/en/test/route'],
});

expect(invalidFunctions.size).toEqual(0);

expect(ignoredFunctions.size).toEqual(1);
expect(
ignoredFunctions.has(resolve(functionsDir, 'en/test/route.func')),
).toEqual(true);
});

test('should not ignore i18n with no valid alternative', async () => {
const processExitMock = vi
.spyOn(process, 'exit')
.mockImplementation(async () => undefined as never);
const mockedConsole = mockConsole('error');

const { collectedFunctions, restoreFsMock } = await collectFunctionsFrom({
functions: {
'en.func': nodejsFuncDir,
},
});

await processEdgeFunctions(collectedFunctions);
await checkInvalidFunctions(collectedFunctions, {
functionsDir,
vercelConfig: {
version: 3,
routes: [{ src: '/(?<nextLocale>fr|en|nl)(/.*|$)' }],
},
});
restoreFsMock();

const { edgeFunctions, invalidFunctions, ignoredFunctions } =
collectedFunctions;

expect(edgeFunctions.size).toEqual(0);

expect(invalidFunctions.size).toEqual(1);
expect(invalidFunctions.has(resolve(functionsDir, 'en.func'))).toEqual(
true,
);

expect(ignoredFunctions.size).toEqual(0);

expect(processExitMock).toHaveBeenCalledWith(1);
mockedConsole.expectCalls([
/The following routes were not configured to run with the Edge Runtime(?:.|\n)+- \/en/,
]);

processExitMock.mockRestore();
mockedConsole.restore();
});
});
Loading
Loading