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(@astrojs/vercel): slowness and symbolic link #8348

Merged
merged 1 commit into from
Sep 1, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 6 additions & 0 deletions .changeset/proud-forks-rescue.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@astrojs/vercel': patch
---

- Cache result during bundling, to speed up the process of multiple functions;
- Avoid creating multiple symbolic links of the dependencies when building the project with `funcitonPerRoute` enabled;
13 changes: 8 additions & 5 deletions packages/integrations/vercel/src/lib/fs.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type { PathLike } from 'node:fs';
import * as fs from 'node:fs/promises';
import { existsSync } from 'node:fs';
import nodePath from 'node:path';
import { fileURLToPath } from 'node:url';

Expand Down Expand Up @@ -74,11 +75,13 @@ export async function copyFilesToFunction(

if (isSymlink) {
const realdest = fileURLToPath(new URL(nodePath.relative(commonAncestor, realpath), outDir));
await fs.symlink(
nodePath.relative(fileURLToPath(new URL('.', dest)), realdest),
dest,
isDir ? 'dir' : 'file'
);
const target = nodePath.relative(fileURLToPath(new URL('.', dest)), realdest);
// NOTE: when building function per route, dependencies are linked at the first run, then there's no need anymore to do that once more.
// So we check if the destination already exists. If it does, move on.
Comment on lines +79 to +80
Copy link
Member

Choose a reason for hiding this comment

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

But doesn't every function get its own node_modules?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but inside a function we have symbolic links:

Screenshot 2023-09-01 at 13 16 08

This is pnpm doing its own thing I guess

Copy link
Member Author

Choose a reason for hiding this comment

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

Full disclosure, it's still something a bit foggy to me, especially that part of the code. It's possible my comment is wrong

// Symbolic links here are usually dependencies and not user code. Symbolic links exist because of the pnpm strategy.
if (!existsSync(dest)) {
await fs.symlink(target, dest, isDir ? 'dir' : 'file');
}
} else if (!isDir) {
await fs.copyFile(origin, dest);
}
Expand Down
32 changes: 21 additions & 11 deletions packages/integrations/vercel/src/lib/nft.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,28 @@
import { relative as relativePath } from 'node:path';
import { fileURLToPath } from 'node:url';
import { relative } from 'node:path';
import { copyFilesToFunction } from './fs.js';
import type { AstroIntegrationLogger } from 'astro';

export async function copyDependenciesToFunction({
entry,
outDir,
includeFiles,
excludeFiles,
}: {
entry: URL;
outDir: URL;
includeFiles: URL[];
excludeFiles: URL[];
}): Promise<{ handler: string }> {
export async function copyDependenciesToFunction(
{
entry,
outDir,
includeFiles,
excludeFiles,
logger,
}: {
entry: URL;
outDir: URL;
includeFiles: URL[];
excludeFiles: URL[];
logger: AstroIntegrationLogger;
},
// we want to pass the caching by reference, and not by value
cache: object
): Promise<{ handler: string }> {
const entryPath = fileURLToPath(entry);
logger.info(`Bundling function ${relative(fileURLToPath(outDir), entryPath)}`);

// Get root of folder of the system (like C:\ on Windows or / on Linux)
let base = entry;
Expand All @@ -31,6 +40,7 @@ export async function copyDependenciesToFunction({
// If you have a route of /dev this appears in source and NFT will try to
// scan your local /dev :8
ignore: ['/dev/**'],
cache,
});

for (const error of result.warnings) {
Expand Down
40 changes: 29 additions & 11 deletions packages/integrations/vercel/src/serverless/adapter.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import type { AstroAdapter, AstroConfig, AstroIntegration, RouteData } from 'astro';
import type {
AstroAdapter,
AstroConfig,
AstroIntegration,
RouteData,
AstroIntegrationLogger,
} from 'astro';
import { AstroError } from 'astro/errors';
import glob from 'fast-glob';
import { basename } from 'node:path';
Expand Down Expand Up @@ -78,16 +84,27 @@ export default function vercelServerless({
// Extra files to be merged with `includeFiles` during build
const extraFilesToInclude: URL[] = [];

async function createFunctionFolder(funcName: string, entry: URL, inc: URL[]) {
const NTF_CACHE = Object.create(null);

async function createFunctionFolder(
funcName: string,
entry: URL,
inc: URL[],
logger: AstroIntegrationLogger
) {
const functionFolder = new URL(`./functions/${funcName}.func/`, _config.outDir);

// Copy necessary files (e.g. node_modules/)
const { handler } = await copyDependenciesToFunction({
entry,
outDir: functionFolder,
includeFiles: inc,
excludeFiles: excludeFiles?.map((file) => new URL(file, _config.root)) || [],
});
const { handler } = await copyDependenciesToFunction(
{
entry,
outDir: functionFolder,
includeFiles: inc,
excludeFiles: excludeFiles?.map((file) => new URL(file, _config.root)) || [],
logger,
},
NTF_CACHE
);

// Enable ESM
// https://aws.amazon.com/blogs/compute/using-node-js-es-modules-and-top-level-await-in-aws-lambda/
Expand Down Expand Up @@ -167,7 +184,7 @@ You can set functionPerRoute: false to prevent surpassing the limit.`
}
},

'astro:build:done': async ({ routes }) => {
'astro:build:done': async ({ routes, logger }) => {
// Merge any includes from `vite.assetsInclude
if (_config.vite.assetsInclude) {
const mergeGlobbedIncludes = (globPattern: unknown) => {
Expand All @@ -192,7 +209,7 @@ You can set functionPerRoute: false to prevent surpassing the limit.`
if (_entryPoints.size) {
for (const [route, entryFile] of _entryPoints) {
const func = basename(entryFile.toString()).replace(/\.mjs$/, '');
await createFunctionFolder(func, entryFile, filesToInclude);
await createFunctionFolder(func, entryFile, filesToInclude, logger);
routeDefinitions.push({
src: route.pattern.source,
dest: func,
Expand All @@ -202,7 +219,8 @@ You can set functionPerRoute: false to prevent surpassing the limit.`
await createFunctionFolder(
'render',
new URL(serverEntry, buildTempFolder),
filesToInclude
filesToInclude,
logger
);
routeDefinitions.push({ src: '/.*', dest: 'render' });
}
Expand Down