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.

Sign up for GitHub

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

Jump to bottom

feat(vercel): streaming #8879

Merged
merged 8 commits into from
Nov 7, 2023
Merged

feat(vercel): streaming #8879

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8e49b35
feat(vercel): streaming
lilnasy Oct 20, 2023
fbc595b
add test
lilnasy Oct 30, 2023
441ae06
add changeset
lilnasy Oct 30, 2023
3a03091
add docs
lilnasy Oct 30, 2023
463f157
fix unrelated test that never ran until this PR for some reason
lilnasy Oct 30, 2023
fe28750
Apply suggestions from code review
lilnasy Nov 1, 2023
d23967a
enabled without configuration
lilnasy Nov 1, 2023
b05a2ec
major -> minor
lilnasy Nov 6, 2023
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
16 changes: 16 additions & 0 deletions .changeset/tricky-clocks-end.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
---
'@astrojs/vercel': minor
---

The Vercel adapter now allows you to enable streaming!

Bring better performance to your visitors by showing them content as it is rendered. The browser can also start loading the required stylesheets and scripts much sooner, which ultimately results in faster full page loads.


```diff
export default defineConfig({
output: "server",
adapter: vercel({
+ streaming: true
}),
});
22 changes: 22 additions & 0 deletions packages/integrations/vercel/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,28 @@ export default defineConfig({
});
```

### streaming

**Type:** `boolean`<br>
**Available for:** Serverless

Determines whether or not the serverless function will stream its response to the browser, sending components as they are rendered.

The default value is `false`. Set this value to `true` in your Vercel adapter configuration to enable streaming.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Determines whether or not the serverless function will stream its response to the browser, sending components as they are rendered.
The default value is `false`. Set this value to `true` in your Vercel adapter configuration to enable streaming.
Use this property to enable Astro's HTML streaming.

I kind of want something like this now, because the docs really do imply this happens by default. (And yes, this seems to be more consistent with the others.)

What do you think about something like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm it won't be limited to HTML. Although the non-HTML cases would be where the user is creating their own streaming responses from scratch.

Copy link
Member

@sarah11918 sarah11918 Nov 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

OK, so I'll tell you where my little stumbling blocks are and YOU propose what works! 😄

Grammatically:

  • "the serverless function" sounds jarring out of place. I never like a "the" if we haven't defined something yet. If you can be more descriptive and avoid the implied context of a "the", that would go a long way.

Philosophically:

  • I really don't like that regular docs say that we do HTML streaming and apparently, sometimes we don't! So, I'd be happier from a narrative standpoint if this default were set to true (can we do that? can we build it that way?) and then this configuration turns it OFF. (Note: even if this feature builds the ability to turn it on, maybe that doesn't have to be the way the feature is USED by the user?) Then the story matches the other adapters, and we can say that you CAN turn it off to do x, y, and z if we want to give use cases that are not... just how our stuff is supposed to work? I feel we can get away with being less define-y here in that case, and say you can remove and either not use, roll your own etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, the serverless function is the particular one created when you deploy to Vercel, at least intended to be that one.

So, I'd be happier from a narrative standpoint if this default were set to true

I agree, it's a documented feature. We could probably make a major release to change the default very soon (but not right away), so that streaming is always the case. And I think it is a good idea for another major release after to remove the option altogether.

Then the story matches the other adapters

Netlify never streams either 😶

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can we build it that way?
Yes :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is now built that way. Streaming with no configuration to opt-out, just the way it should be.

Copy link
Member

Choose a reason for hiding this comment

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

Netlify never streams either 😶

What even is happening?? The cake is a lie! 😅

So, I don't want anyone's project to break with this change, of course! Aligning this with docs is helpful for me, since that's the story we tell pretty clearly, but if we've been misinformed then we can change on our end, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Breaking is not a concern, just that the change is going to be more visible than what someone might expect from a minor or patch release. So a major release is important more because the user needs to be given notice about what's happening.


```diff lang="js"
// astro.config.mjs
import { defineConfig } from 'astro/config';
import vercel from '@astrojs/vercel/serverless';

export default defineConfig({
output: "server",
adapter: vercel({
+ streaming: true
}),
});
```

### Function bundling configuration

The Vercel adapter combines all of your routes into a single function by default.
Expand Down
11 changes: 10 additions & 1 deletion packages/integrations/vercel/src/serverless/adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,9 @@ export interface VercelServerlessConfig {

/** The maximum duration (in seconds) that Serverless Functions can run before timing out. See the [Vercel documentation](https://vercel.com/docs/functions/serverless-functions/runtimes#maxduration) for the default and maximum limit for your account plan. */
maxDuration?: number;

/** Whether to allow the serverless function to stream the response to the browser. */
streaming?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

I feel like @sarah11918 might have thoughts on the wording here!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm hoping lol

Copy link
Member

Choose a reason for hiding this comment

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

I want to see which direction the README takes before bringing that over here! Don't let me forget about this, though!

}

export default function vercelServerless({
Expand All @@ -119,6 +122,7 @@ export default function vercelServerless({
functionPerRoute = false,
edgeMiddleware = false,
maxDuration,
streaming,
}: VercelServerlessConfig = {}): AstroIntegration {
if (maxDuration) {
if (typeof maxDuration !== 'number') {
Expand Down Expand Up @@ -276,6 +280,7 @@ You can set functionPerRoute: false to prevent surpassing the limit.`
includeFiles: filesToInclude,
excludeFiles,
maxDuration,
streaming,
});
routeDefinitions.push({
src: route.pattern.source,
Expand All @@ -292,6 +297,7 @@ You can set functionPerRoute: false to prevent surpassing the limit.`
includeFiles: filesToInclude,
excludeFiles,
maxDuration,
streaming,
});
routeDefinitions.push({ src: '/.*', dest: 'render' });
}
Expand Down Expand Up @@ -341,7 +347,8 @@ interface CreateFunctionFolderArgs {
NTF_CACHE: any;
includeFiles: URL[];
excludeFiles?: string[];
maxDuration?: number;
maxDuration: number | undefined;
streaming: boolean | undefined;
}

async function createFunctionFolder({
Expand All @@ -353,6 +360,7 @@ async function createFunctionFolder({
includeFiles,
excludeFiles,
maxDuration,
streaming,
}: CreateFunctionFolderArgs) {
const functionFolder = new URL(`./functions/${functionName}.func/`, config.outDir);

Expand Down Expand Up @@ -381,6 +389,7 @@ async function createFunctionFolder({
handler,
launcherType: 'Nodejs',
maxDuration,
supportsResponseStreaming: streaming,
});
}

Expand Down
9 changes: 9 additions & 0 deletions packages/integrations/vercel/test/fixtures/streaming/astro.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { defineConfig } from 'astro/config';
import vercel from '@astrojs/vercel/serverless';

export default defineConfig({
output: "server",
adapter: vercel({
streaming: true
})
});
10 changes: 10 additions & 0 deletions packages/integrations/vercel/test/fixtures/streaming/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"name": "@test/vercel-streaming",
"version": "0.0.0",
"private": true,
"dependencies": {
"@astrojs/vercel": "workspace:*",
"astro": "workspace:*"
}
}

8 changes: 8 additions & 0 deletions packages/integrations/vercel/test/fixtures/streaming/src/pages/one.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<html>
<head>
<title>One</title>
</head>
<body>
<h1>One</h1>
</body>
</html>
8 changes: 8 additions & 0 deletions packages/integrations/vercel/test/fixtures/streaming/src/pages/two.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<html>
<head>
<title>Two</title>
</head>
<body>
<h1>Two</h1>
</body>
</html>
15 changes: 8 additions & 7 deletions packages/integrations/vercel/test/static-assets.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@ describe('Static Assets', () => {

const VALID_CACHE_CONTROL = 'public, max-age=31536000, immutable';

async function build({ adapter, assets }) {
async function build({ adapter, assets, output }) {
fixture = await loadFixture({
root: './fixtures/static-assets/',
output,
adapter,
build: {
assets,
Expand Down Expand Up @@ -38,31 +39,31 @@ describe('Static Assets', () => {
}

describe('static adapter', async () => {
const adapter = await import('@astrojs/vercel/static');
const { default: vercel } = await import('@astrojs/vercel/static');

it('has cache control', async () => {
await build({ adapter });
await build({ adapter: vercel() });
checkValidCacheControl();
});

it('has cache control other assets', async () => {
const assets = '_foo';
await build({ adapter, assets });
await build({ adapter: vercel(), assets });
checkValidCacheControl(assets);
});
});

describe('serverless adapter', async () => {
const adapter = await import('@astrojs/vercel/serverless');
const { default: vercel } = await import('@astrojs/vercel/serverless');

it('has cache control', async () => {
await build({ adapter });
await build({ output: "server", adapter: vercel() });
checkValidCacheControl();
});

it('has cache control other assets', async () => {
const assets = '_foo';
await build({ adapter, assets });
await build({ output: "server", adapter: vercel(), assets });
checkValidCacheControl(assets);
});
});
Expand Down
21 changes: 21 additions & 0 deletions packages/integrations/vercel/test/streaming.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import { loadFixture } from './test-utils.js';
import { expect } from 'chai';

describe('maxDuration', () => {
/** @type {import('./test-utils.js').Fixture} */
let fixture;

before(async () => {
fixture = await loadFixture({
root: './fixtures/streaming/',
});
await fixture.build();
});

it('makes it to vercel function configuration', async () => {
const vcConfig = JSON.parse(
await fixture.readFile('../.vercel/output/functions/render.func/.vc-config.json')
);
expect(vcConfig).to.deep.include({ supportsResponseStreaming: true });
});
});
9 changes: 9 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading