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

feat: include server assets for Vercel serverless functions #10979

Merged
merged 66 commits into from
Jan 15, 2024

Conversation

eltigerchino
Copy link
Member

@eltigerchino eltigerchino commented Nov 4, 2023

fixes most of #10594

This PR extends the builder route metadata with a list of server assets used by each route. The vercel adapter uses this to copy only the server assets each serverless function needs, addressing the concern in #8441 (review)

I'd like some thoughts and to get some help with improving the docs for this as I think there can be a better example and description.

One concern that I have is the Vercel function size growing due to the automatic bundling of assets. Maybe we can choose to exclude page and layout component files? Those technically run on the server during SSR but we probably don’t want to bundle the font, images, etc. that are usually meant for the client only.
https://vercel.com/docs/functions/serverless-functions/runtimes#bundle-size-limits

EDIT: We will only include assets imported in server-only files such as +page.server, +layout.server, +server, and hooks.server (and any transitive imports). Universal load and components are automatically excluded.

TODO:

  • Need to find out how Netlify serverless functions handles files then update the Netlify adapter. Maybe this can come later? The Netlify adapter already copies all server assets over, we just need to document this.
  • The Node adapter should be more straight forward...? (just copy all server assets to the root of the build).
  • Add build output tests for the vercel adapter
  • I think the root layout and error page's server assets need to be included with the fallback lambda
  • include root layout and root error page assets in every route
  • Check for non-root error pages to see if they use any server assets?
  • Include the assets used by layout components (the layout.svelte files).

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Copy link

changeset-bot bot commented Nov 4, 2023

🦋 Changeset detected

Latest commit: 9f03b6b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@sveltejs/adapter-vercel Minor
@sveltejs/kit Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@eltigerchino eltigerchino marked this pull request as draft November 4, 2023 16:10
Copy link
Member

@antony antony left a comment

Choose a reason for hiding this comment

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

Tiny code feedback, the docs look fine here if not a little convoluted for the use case. Otherwise the PR is great, this would be a great addition to SK.

const file = `/tmp/${filename}.pdf`;
let writeStream = fs.createWriteStream(file);
doc.pipe(writeStream);
doc.font(font).fontSize(25).text(title, 100, 100);
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 use-case for having a font rendered with a PDF is great, but perhaps something simpler like just serving up a .txt file is more concise example. This example sort-of requires understanding of how PDFs are rendered, and includes both streaming code and use of temporary intermediate files (which is also good to have documented somewhere, but conflating the two concepts here might be a bit much).

Copy link
Member Author

Choose a reason for hiding this comment

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

Vite inlines the imported text file so it doesn’t get copied as an asset. I couldn’t think of another example and followed the one provided by Vercel. I’ll try to think up of something else

Copy link
Member

Choose a reason for hiding this comment

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

Could you just not import it? My use case was that i'm compiling mjml - I import the templates with ?raw which is great, but the mjml compiler needs to know about an "includes" dir which it grabs files from when it compiles the mjml to html. I spent hours, and hours trying to get this to work on Vercel.

As I mentioned before it might be esoteric, but I'm sure there is another use-case - I've struggled with static files before when trying to grab compiled runtimes out of the "sharp" package and forcing them to deploy to Vercel as it does an fs.readFile at runtime to grab the compiled bundle.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it has to be imported so Vite’s asset handling kicks in and we grab the processed asset urls off the vite manifest https://vitejs.dev/guide/assets could you share a code example of how your use case is being implemented?

Copy link
Member

@antony antony Jan 15, 2024

Choose a reason for hiding this comment

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

Interesting - so I guess my question is, would this work if I wanted to include a directory full of assets, without specifically importing each one. My code looks like this:

// postbuild.js <- run after `vercel build` to deposit files into output dir
import { cp, mkdir } from 'node:fs/promises'
import { join } from 'node:path'

const includesDir = join(
'.',
'.vercel',
'output',
'functions',
'fn.func',
'apps',
'comms',
'includes',
)

const options = { recursive: true }

Promise.all([
  mkdir(includesDir, options),
  cp(
    join('node_modules', '@beyonk', 'email', 'lib', 'includes'),
    includesDir,
    options
  )
])

and then in my code I can reference this dir so that the mjml knows where to find its files. No imports required.

const includesDir = join(process.cwd(), 'apps', 'comms', 'includes')

function compile (template, substitutions) {
  const { html: htmlSource, errors } = toHtml(template.mjml, {
    filePath: includesDir
  })
})

Copy link
Member Author

@eltigerchino eltigerchino Jan 15, 2024

Choose a reason for hiding this comment

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

Yeah, that post-build script should work great #10594 (comment) . Someone tried something similar here https://github.com/syntaxfm/website/pull/1179/files#diff-d3309f83fe6f5a74a8d1dde6c819338f4550d93f9308702c018eeb3692955f14 This PR is more of convenience, and especially if split functions are enabled.

You could also use the glob import to get multiple file paths https://vitejs.dev/guide/features.html#glob-import

Copy link
Member

Choose a reason for hiding this comment

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

The post-build feels brittle, but I guess until there is more demand for this sort of thing, it's a fine solution.

packages/adapter-vercel/index.js Outdated Show resolved Hide resolved
packages/adapter-vercel/index.js Outdated Show resolved Hide resolved
@eltigerchino eltigerchino merged commit fe1a11e into main Jan 15, 2024
13 checks passed
@eltigerchino eltigerchino deleted the feat-server-assets branch January 15, 2024 12:50
@github-actions github-actions bot mentioned this pull request Jan 12, 2024
@eltigerchino eltigerchino mentioned this pull request Jan 15, 2024
6 tasks
Comment on lines +126 to +127
// server assets live in `.netlify/server` when deployed to Netlify
const dir = dev ? cwd : path.join(cwd, '.netlify/server');
Copy link
Member

Choose a reason for hiding this comment

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

This feels very hacky. Would love it if we could find a neater solution

Copy link
Member

Choose a reason for hiding this comment

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

This, for Netlify is approximately the same as the official documented Vercel solution - if you use next you can yomp the cwd and a magical '/../' path together, but if you don't, you have to use __dirname + string concatenation. Neither work on kit, so I went back to join(process.cwd(), /foo/bar/baz).

and locally of course it's all completely different anyway, since these are built-time post-adapter generated dirs

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'll revert this documentation change until there's a better solution for Netlify.

Comment on lines +383 to +385
if (config.runtime !== 'edge') {
images = /** @type {import('./index.js').ServerlessConfig} */ (config).images;
}
Copy link
Member

Choose a reason for hiding this comment

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

what's this change about?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah I was trying to resolve some type issues in the adapter but I didn't fully revert those changes. I'll revert them properly and defer them to another PR

@@ -189,6 +199,124 @@ export function create_builder({
return build_data.app_path;
},

getServerAssets() {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure about the naming of this method — the other getFoo methods just return a directory name. The generateFoo methods are slightly inconsistent with each other since some write a file to disk while generateManifest returns the contents of a manifest module, but the prefix feels slightly more appropriate since it's doing some work rather than just concatenating strings

Relatedly (since it's a question of API design) I'm still unclear on why rootErrorPage needs to be handled separately — it almost feels like it should be something like this

generateAssetList(routes): string[]

though it would be worth thinking through how it will be used by other adapters, to figure out if that makes sense

Copy link
Member Author

Choose a reason for hiding this comment

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

How about generateServerAssetList?

@eltigerchino eltigerchino restored the feat-server-assets branch January 16, 2024 00:48
@eltigerchino eltigerchino deleted the feat-server-assets branch January 16, 2024 00:49
@Rich-Harris Rich-Harris mentioned this pull request Jan 16, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:adapter-vercel Pertaining to the Vercel adapter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants