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(vercel): edge middleware #9585

Merged
merged 12 commits into from
Jan 22, 2024
Merged

Conversation

lilnasy
Copy link
Contributor

@lilnasy lilnasy commented Jan 2, 2024

Changes

Testing

Added cases to edge-middleware.test.js

Docs

Does not affect usage.

Copy link

changeset-bot bot commented Jan 2, 2024

🦋 Changeset detected

Latest commit: 930f851

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

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

@github-actions github-actions bot added pkg: integration Related to any renderer integration (scope) pkg: astro Related to the core `astro` package (scope) docs pr A PR that includes documentation for review labels Jan 2, 2024
@sarah11918 sarah11918 removed the docs pr A PR that includes documentation for review label Jan 9, 2024
@gislerro

This comment has been minimized.

@lilnasy
Copy link
Contributor Author

lilnasy commented Jan 15, 2024

@gislerro Will be ready for review soon after astro 4.2 (wednesday). The work is done, it is just based on a future branch.

Late this week/early next week.

@github-actions github-actions bot removed the pkg: astro Related to the core `astro` package (scope) label Jan 17, 2024
remove getVercelOutput
@lilnasy lilnasy marked this pull request as ready for review January 18, 2024 00:10
Comment on lines +348 to +372
interface CreateMiddlewareFolderArgs {
config: AstroConfig
entry: URL
functionName: string
}

async function createMiddlewareFolder({
functionName,
entry,
config,
}: CreateMiddlewareFolderArgs) {
const functionFolder = new URL(`./functions/${functionName}.func/`, config.outDir);

await generateEdgeMiddleware(
entry,
new URL(VERCEL_EDGE_MIDDLEWARE_FILE, config.srcDir),
new URL('./middleware.mjs', functionFolder),
)

await writeJson(new URL(`./.vc-config.json`, functionFolder), {
runtime: 'edge',
entrypoint: 'middleware.mjs',
});
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We create an edge function, complete with its own folder and vc-config.

Comment on lines 73 to 81
const { origin } = new URL(request.url);
const next = () =>
fetch(new URL('/_render', request.url), {
headers: {
${JSON.stringify(ASTRO_LOCALS_HEADER)}: trySerializeLocals(ctx.locals)
...Object.fromEntries(request.headers.entries()),
'x-astro-path': request.url.replace(origin, ''),
'x-astro-locals': trySerializeLocals(ctx.locals)
}
});
return response;
};
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The edge function calls the node server at /_render, with the original path as a header.

Comment on lines 316 to 317
: _middlewareEntryPoint ? '_middleware'
: 'render',
Copy link
Member

Choose a reason for hiding this comment

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

Should this use dest instead? Or the render should be _render

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 we should move this strings (_render, _middleware) to some constants, so we don't do any typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, it is supposed to be _render. Moved to constants.

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Sorry to bother you, but this PR has many changes that aren't related to the edge middleware and aren't explained in the PR description.

Could you:

  1. Describe the entity to the changes in the PR description. For example, we don't know why you changed render to _render.
  2. Move them in a separate PR

}
}
if (_middlewareEntryPoint) {
await createMiddlewareFolder({
functionName: '_middleware',
Copy link
Member

Choose a reason for hiding this comment

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

Why _middleware and not middleware?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Vercel routes the folder names to a path on the deployed website so we avoid interfering this way.

Comment on lines 316 to 317
: _middlewareEntryPoint ? '_middleware'
: 'render',
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 we should move this strings (_render, _middleware) to some constants, so we don't do any typo

Comment on lines 13 to 16
const realPath = req.headers['x-astro-path'];
if (typeof realPath === 'string') {
req.url = realPath;
}
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
Contributor Author

Choose a reason for hiding this comment

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

This is so that the edge function can call the serverless function in next().

Copy link
Member

Choose a reason for hiding this comment

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

I still think it's worth adding a comment because the read and write of the header happens in two different places, so it could be helpful to give some context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lilnasy lilnasy merged commit 05adaaa into withastro:main Jan 22, 2024
13 checks passed
@lilnasy lilnasy deleted the vercel-edge-middleware branch January 22, 2024 23:34
@astrobot-houston astrobot-houston mentioned this pull request Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Middleware works locally, doesn't run at all when deployed to Vercel
6 participants