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

serhalp · 2024-03-28T20:50:27Z

When using edge functions with Netlify, the generated edge function was configured to run against all paths. This conflicted with the expectation that paths under .netlify/ (such as .netlify/identity/settings) are ignored by the framework.

At the time #5235 was opened, this was difficult to solve, but Netlify now supports path exclusions in edge function configuration, hence this PR.

While I was here, I also added an exclusion to avoid unnecessarily invoking this edge function on static files. Currently it's just a no-op, but it's still nice to avoid the unnecessary invocation.

Testing this is a bit involved, but I was able to repro the issue in #5235 and to confirm this fixes it. We should probably add proper integration tests here eventually.

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:.

Edits

Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

Previously Netlify Edge Functions didn't support a way to configure path matching that *excludes* paths. Now that it does, we can avoid running the "render" edge function on static assets. Currently, it's [just a no-op](https://github.com/sveltejs/kit/blob/80386437030c5c79913e859e3c32fd194613e1b6/packages/adapter-netlify/src/edge.js#L18-L22), but it's still nice to avoid the invocation.

changeset-bot · 2024-03-28T20:50:31Z

🦋 Changeset detected

Latest commit: 42bab13

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

This PR includes changesets to release 1 package

Name	Type
@sveltejs/adapter-netlify	Patch

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

Fixes sveltejs#5235.

serhalp · 2024-03-28T21:23:53Z

packages/adapter-netlify/index.js

+ * TODO(serhalp) Replace this custom type with an import from `@netlify/edge-functions`,
+ * once that type is fixed to include `excludedPath` and `function`.


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

serhalp · 2024-04-22T22:42:57Z

Hi @benmccann, nice to meet you! Is there anything I need to do to get this PR merged and released? Please let me know. Thanks!

…erate-edge-functions-that-dont

dummdidumm

Sorry for leaving this without a review for so long! From a first glance it looks good. One questions though: What happens if paths.base (SvelteKit) is set, does that have any influence on this (or in other words would it break)?

@eltigerchino can you have another look at this (checking what happens with a base path, and also investigate whether we can remove the code in is_static_file?

dummdidumm · 2025-01-13T12:47:13Z

packages/adapter-netlify/index.js

@@ -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`.


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

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.

packages/adapter-netlify/src/edge.js

.changeset/witty-teachers-film.md

eltigerchino · 2025-01-14T06:16:33Z

Having a base path seems to work fine with this PR.

eltigerchino · 2025-01-15T08:15:35Z

packages/adapter-netlify/index.js

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


Wonder if we should add the base path to this:

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

dummdidumm · 2025-01-15T13:10:29Z

packages/adapter-netlify/index.js

 	const excludedPath = [
 		// Contains static files
 		`/${builder.getAppPath()}/*`,
+		...builder.prerendered.paths,
+		...Array.from(assets).flatMap((asset) => {


the previous logic also checked within manifest._.server_assets, this one does not - oversight or on purpose?

We could add manifest._.server_assets too but they're currently already included by the immutable assets exclusion /${builder.getAppPath()}/* because we copy the server assets over and make them public along with the client assets during the build process. I'm not sure if that will change in the future.

kit/packages/kit/src/exports/vite/index.js

Lines 848 to 851 in f451f6c

copy(

`${out}/server/${kit.appDir}/immutable/assets`,

`${out}/client/${kit.appDir}/immutable/assets`

);

fix(netlify): don't try to render .netlify/*

5c3e74a

Fixes sveltejs#5235.

serhalp force-pushed the serhalp/fra-186-sveltekit-adapter-should-generate-edge-functions-that-dont branch from e87b06a to 5c3e74a Compare March 28, 2024 21:21

serhalp marked this pull request as ready for review March 28, 2024 21:23

serhalp commented Mar 28, 2024

View reviewed changes

benmccann added the pkg:adapter-netlify label Apr 1, 2024

eltigerchino changed the title ~~fix(netlify): fix conflict between Netlify Identity and edge function rendering~~ fix: prevent conflict between Netlify Identity and edge function rendering May 16, 2024

vercel bot deployed to Preview – kit-svelte-dev May 16, 2024 16:24 View deployment

vercel bot deployed to Preview – kit-default-template May 16, 2024 16:24 View deployment

dummdidumm added 2 commits January 13, 2025 13:40

Merge branch 'main' into serhalp/fra-186-sveltekit-adapter-should-gen…

902ea7a

…erate-edge-functions-that-dont

adjust changeset

beacaae

dummdidumm reviewed Jan 13, 2025

View reviewed changes

Merge branch 'main' into pr/serhalp/12052

a63c563

eltigerchino reviewed Jan 14, 2025

View reviewed changes

.changeset/witty-teachers-film.md Outdated Show resolved Hide resolved

Update .changeset/witty-teachers-film.md

2d81ef4

eltigerchino reviewed Jan 15, 2025

View reviewed changes

replace is_static_file check with excluded paths

75fce73

dummdidumm reviewed Jan 15, 2025

View reviewed changes

eltigerchino added 2 commits January 16, 2025 10:36

remove prerendered type declaration

09e42c6

Merge branch 'main' into pr/serhalp/12052

42bab13

dummdidumm approved these changes Jan 21, 2025

View reviewed changes

dummdidumm merged commit 0d813dc into sveltejs:main Jan 21, 2025
14 checks passed

github-actions bot mentioned this pull request Jan 21, 2025

Version Packages #13358

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

serhalp commented Mar 28, 2024 •

edited

Loading

changeset-bot bot commented Mar 28, 2024 •

edited

Loading

serhalp Mar 28, 2024

serhalp commented Apr 22, 2024

dummdidumm left a comment •

edited

Loading

dummdidumm Jan 13, 2025

eltigerchino Jan 24, 2025

eltigerchino commented Jan 14, 2025

eltigerchino Jan 15, 2025

dummdidumm Jan 15, 2025

eltigerchino Jan 16, 2025 •

edited

Loading

		* TODO(serhalp) Replace this custom type with an import from `@netlify/edge-functions`,
		* once that type is fixed to include `excludedPath` and `function`.

	copy(
	`${out}/server/${kit.appDir}/immutable/assets`,
	`${out}/client/${kit.appDir}/immutable/assets`
	);

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

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

Conversation

serhalp commented Mar 28, 2024 • edited Loading

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

Tests

Changesets

Edits

changeset-bot bot commented Mar 28, 2024 • edited Loading

🦋 Changeset detected

serhalp Mar 28, 2024

Choose a reason for hiding this comment

serhalp commented Apr 22, 2024

dummdidumm left a comment • edited Loading

Choose a reason for hiding this comment

dummdidumm Jan 13, 2025

Choose a reason for hiding this comment

eltigerchino Jan 24, 2025

Choose a reason for hiding this comment

eltigerchino commented Jan 14, 2025

eltigerchino Jan 15, 2025

Choose a reason for hiding this comment

dummdidumm Jan 15, 2025

Choose a reason for hiding this comment

eltigerchino Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

serhalp commented Mar 28, 2024 •

edited

Loading

changeset-bot bot commented Mar 28, 2024 •

edited

Loading

dummdidumm left a comment •

edited

Loading

eltigerchino Jan 16, 2025 •

edited

Loading