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

fix: prevent conflict between Netlify Identity and edge function rendering #12052

Merged
merged 9 commits into from
Jan 21, 2025
Merged

fix: prevent conflict between Netlify Identity and edge function rendering #12052

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
aed3284
feat(netlify): don't run no-op edge function on static assets
serhalp Mar 28, 2024
5c3e74a
fix(netlify): don't try to render .netlify/*
serhalp Mar 28, 2024
902ea7a
Merge branch 'main' into serhalp/fra-186-sveltekit-adapter-should-gen…
dummdidumm Jan 13, 2025
beacaae
adjust changeset
dummdidumm Jan 13, 2025
a63c563
Merge branch 'main' into pr/serhalp/12052
eltigerchino Jan 14, 2025
2d81ef4
Update .changeset/witty-teachers-film.md
eltigerchino Jan 14, 2025
75fce73
replace `is_static_file` check with excluded paths
eltigerchino Jan 15, 2025
09e42c6
remove prerendered type declaration
eltigerchino Jan 16, 2025
42bab13
Merge branch 'main' into pr/serhalp/12052
eltigerchino Jan 21, 2025
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
5 changes: 5 additions & 0 deletions .changeset/witty-teachers-film.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@sveltejs/adapter-netlify": patch
---

Fix conflict between Netlify Edge Functions and Netlify Identity. Avoid unnecessary Netlify edge function invocations for static files.
19 changes: 13 additions & 6 deletions packages/adapter-netlify/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,19 @@ import toml from '@iarna/toml';
*/

/**
* TODO(serhalp) Replace this custom type with an import from `@netlify/edge-functions`,
* once that type is fixed to include `excludedPath` and `function`.
Comment on lines +17 to +18
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI I'm working on this as well. I should have a follow-up PR soon.

Copy link
Member

Choose a reason for hiding this comment

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

I see that @netlify/edge-functions does contain excludedPattern now but not function - so it looks like we can't use that still.

Copy link
Member

Choose a reason for hiding this comment

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

Looking at https://www.npmjs.com/package/@netlify/edge-functions?activeTab=code the ManifestFunction type contains the correct definition if we want to use those types.

* @typedef {{
* functions: Array<
* | {
* function: string;
* path: string;
* excludedPath?: string | string[];
* }
* | {
* function: string;
* pattern: string;
* excludedPattern?: string | string[];
* }
* >;
* version: 1;
Expand Down Expand Up @@ -121,18 +125,21 @@ async function generate_edge_functions({ builder }) {

builder.mkdirp('.netlify/edge-functions');

// Don't match the static directory
const pattern = '^/.*$';

// Go doesn't support lookarounds, so we can't do this
// const pattern = appDir ? `^/(?!${escapeStringRegexp(appDir)}).*$` : '^/.*$';
const path = '/*';
Copy link
Member

Choose a reason for hiding this comment

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

Wonder if we should add the base path to this:

const path = `${builder.config.kit.paths.base}/*`;

const excludedPath = [
// Contains static files
`/${builder.getAppPath()}/*`,
// Should not be served by SvelteKit at all
'/.netlify/*'
];

/** @type {HandlerManifest} */
const edge_manifest = {
functions: [
{
function: 'render',
pattern
path,
excludedPath
}
],
version: 1
Expand Down
2 changes: 2 additions & 0 deletions packages/adapter-netlify/src/edge.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ function is_static_file(request) {
const url = new URL(request.url);

// Assets in the app dir
// TODO(serhalp) We no longer run the edge function on these at all. Can we remove this check?
eltigerchino marked this conversation as resolved.
Show resolved Hide resolved
if (url.pathname.startsWith(prefix)) {
return true;
}
Expand All @@ -51,6 +52,7 @@ function is_static_file(request) {
// ignore
}

// TODO(serhalp) Now that edge function config supports `excludedPath`, could we replace all these checks?
eltigerchino marked this conversation as resolved.
Show resolved Hide resolved
return (
manifest.assets.has(file) ||
manifest.assets.has(file + '/index.html') ||
Expand Down
Loading