-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
🦋 Changeset detectedLatest commit: 9f03b6b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
There was a problem hiding this 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); |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 import
ing 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
})
})
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
// server assets live in `.netlify/server` when deployed to Netlify | ||
const dir = dev ? cwd : path.join(cwd, '.netlify/server'); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
if (config.runtime !== 'edge') { | ||
images = /** @type {import('./index.js').ServerlessConfig} */ (config).images; | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about generateServerAssetList
?
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).Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.