Enhance custom configuration support #66

FlippingBinary · 2022-09-23T20:47:40Z

This pull request fixes #65 by allowing wildcard routes to provide default settings for all routes created by the adapter (rather than preventing wildcard routes). It allows custom routes to set rewrite: 'ssr' to force only dynamic handling of a route or rewrite: undefined to disable it entirely. It allows the custom configuration to define navigationFallback so the exclude rule can be set. It also allows the platform.apiRuntime to be overridden but will throw an error if it is set to a string that doesn't start with node: so it can't be set to something crazy but a different node version can be set. Documentation has been updated to describe the new capabilities, along with a description of what happens when things aren't set correctly. The validation function has been removed because I don't think it's needed anymore.

I have tested this locally and it works as expected.

geoffrich · 2022-09-25T23:21:35Z

Hey, I still need to find time to review this PR more fully. It looks like we're adding a lot of logic, so I just merged #67 which adds unit testing around the existing functionality. It also extracts a dedicated function for constructing the SWA config that we can unit test directly.

Are you able to integrate those changes into your branch and update the unit tests to handle the new code paths? Unit test cases will also help me better understand the different usecases.

FlippingBinary · 2022-09-26T02:22:04Z

Absolutely! I'm glad you added unit testing. I'll merge the changes soon, probably tomorrow, and update this pull request.

FlippingBinary · 2022-09-26T14:34:35Z

@geoffrich I think it's ready for you to take another look at it.

A few things to note:

This code removes the config validation function because it can handle wildcards and navigationFallback. The config does not need a rewrite anywhere, but it is trusted if it appears. Microsoft's documentation uses /* to be a catch-all wildcard, but treats * and /* as equal, so that's what these changes do as well.
I moved the config generation function call to right before it is used because it needs to know if there is a pre-rendered file at /, which isn't known until then, and the return value isn't needed until it is written to a file. Now the generateConfig(...) function is entirely responsible for generating the config. Nothing else changes it.
I switched from .toStrictEqual to .toEqual for testing the config because the strict version tests for differences that do not carry through JSON.stringify so they don't matter anyway.

FlippingBinary · 2022-09-27T13:32:23Z

I'm reviewing this pull request again and just realized I've grown lazy in my reliance on TypeScript! There are a few bugs I need to fix.

Here are a few things I'll do:

Change the generateConfig function signature to generateConfig(builder, customStaticWebAppConfig). The current version still has some code I copied from adapt that expects builder to be there, which I didn't notice because it didn't cause a problem when it failed to run. This change puts the customStaticWebAppConfig in a different position than before, but it's not a public function and the changed order makes it possible to give customStaticWebAppConfig a default value properly.
Remove the CustomStaticWebAppConfig type and replace it with StaticWebAppConfig because it isn't needed anymore.
Add a type hint for the adapter and builder.

Also, right now platform.apiRuntime is forced to be node:16, but perhaps it should allow any node version to be specified. The version specified in the config could then be used when executing esbuild.build. This would help to future-proof the adapter because then projects can override the node version when it's appropriate for their project. I was on the fence about including it, but I think it's pretty clear now that this pull request is about more than just allowing wildcard routes and it feels incomplete without support for overriding platform.apiRuntime.

FlippingBinary · 2022-09-29T16:13:20Z

This could close #48, but I wonder if it should enforce SvelteKit's minimum node version instead of allowing any node version?

geoffrich

Sorry it took me a bit to properly review - there was a lot to wrap my head around in this PR.

I feel like we’re accounting for too many use cases here. The original issue was to allow adding additional settings to the wildcard route, and it feels like this is increasing the scope for use cases that are unclear to me.

In my mind there are three distinct cases where you want to configure custom routes:

You want to add additional settings (e.g. allowedRoles) to all routes, but let SvelteKit handle the rest. This can be enabled by allowing the custom config to set a * route, and we apply the settings to all routes generated by the adapter. We should assume the wildcard route will still be handled by SvelteKit, i.e. we should not require passing “rewrite: ssr” (and should probably error if they try to rewrite the * route)
You want to add additional settings to some routes, e.g. protecting a /admin route, but still have SK handle the rest of the logic. I’m not sure we can entirely handle this via the SWA config, since once you load up the app client-side navigation takes over and routes could be rendered without hitting the network and going through the SWA auth process. This may need to be tackled inside your SK app.
You want to handle certain routes completely outside of SvelteKit. This may just work since SvelteKit will fall back to the network if the link doesn’t match anything.

I think it would be best if we just enable the first use case in this PR for now, and then explore the other two in followups if changes are necessary. I think that will scope down a lot of the changes included in this PR and make it easier to review.

geoffrich · 2022-09-27T15:33:06Z

index.js

+		if (['/*', '*'].includes(route.route)) {
+			route.route = '*';


Is /* equivalent to *? i.e., do they both match https://svelte.dev and https://svelte.dev/docs?

I tested that by setting both routes, but SWA rejected the duplicate.

index.js

geoffrich · 2022-10-05T16:35:03Z

index.js

+	if (swaConfig.navigationFallback.rewrite === ssrTrigger) {
+		swaConfig.navigationFallback.rewrite = ssrFunctionRoute;
+	}


I don't think it makes sense to allow customization of this. This will break the adapter completely. Is there a use case I'm missing?

There are a few use cases when it would not break the site and would be preferable because it prevents usage charges from bots all over the world scanning your site for vulnerabilities at paths that don't exist.

For a fully static site, there is no need for SSR and it makes sense to disable it entirely.

For a site with explicit endpoints, the routes with SSR enabled can be narrowly defined.

And, of course, if the configuration key is not set it cannot break the adapter. It just gives more options for projects that use the adapter.

For 1, the user of the adapter shouldn't need to do anything for that - you configure prerendered pages in SvelteKit and Azure SWA will automatically route to those prerendered pages without hitting the SSR function.

For 2, I'd want to see an example of what the use case is there. navigationFallback is meant to be the final fallback if nothing else resolves the GET request. If you make it anything other than the generated render function, SvelteKit's client-side routing will break completely.

It just gives more options for projects that use the adapter.

I want to avoid exposing an option unless there is a specific use-case to solve for. More options/features means more things to maintain and consider when making other updates to the adapter.

The specific use-case is to prevent 404 errors from racking up Azure function usage charges on routes that are never supposed to be SSR. Yes, you can configure prerendered pages in SvelteKit, but 404 errors will be rendered by an Azure function with this adapter as it is currently built.

I want to avoid exposing an option unless there is a specific use-case to solve for. More options/features means more things to maintain and consider when making other updates to the adapter.

I agree generally, but this code only gets the adapter out of the way of features provided by Azure. It allows the custom configuration to work the way Microsoft's documentation describes, and it has no impact on people who don't use it.

geoffrich · 2022-10-05T16:35:48Z

index.js

+			rewrite: ssrFunctionRoute,
+			...customStaticWebAppConfig.navigationFallback


Unless there's a good reason to allow for customization of navigationFallback.rewrite (see other comment), these lines should be swapped

Suggested change

rewrite: ssrFunctionRoute,

...customStaticWebAppConfig.navigationFallback

...customStaticWebAppConfig.navigationFallback,

rewrite: ssrFunctionRoute

I put it in this order specifically so that custom configuration can override it, if the key is present. I am strongly in favor of being clear about the behavior in the documentation and letting the developer decide what's best for their use-case. It doesn't affect my current use-case and this is your project, so I will commit your suggestion if you feel strongly about it, but I would really like to have the opportunity to make improvements to the documentation until you're satisfied with the results.

Co-authored-by: Geoff Rich <4992896+geoffrich@users.noreply.github.com>

FlippingBinary · 2022-10-05T19:38:33Z

Just to clarify: This pull request does not make it necessary to write rewrite: 'ssr' anywhere. If the key is absent, everything works as it does now.

There are two reasons why someone might need to put rewrite: 'ssr' in a route:

They want to define other properties of a route that uses SSR-only methods like POST or PUT.
They want to disable SSR in navigationRewrite but still have some routes that are SSR.

In my case, I currently have a POST endpoint that has to have a rewrite: '/api/__render' in it or else SWA will reject it. So that's a large part of why I added the 'ssr' to /api/__render conversion. Since /api/__render is an internal value, it might change in the future and it would be easier to do that if projects use a keyword instead.

This is why use case 2 in your list is important.

You make a really good point about SvelteKit slipping past allowedRoles because it doesn't rely on standard network calls for navigation. I think it would be a good feature enhancement for SvelteKit to have the ability to include a hook from the adapter that can bring in route-checking logic to prevent that kind of leak. If that's not possible, the potential for a leak would need to be documented.

I don't have a need for use case 3 in your list right now, but it is already possible so I wouldn't want to interfere with it.

FlippingBinary added 6 commits September 23, 2022 16:41

allow wildcard routes

940f268

Ignore methods of wildcard when creating routes

86f658a

Track methods of routes to avoid conflicts

fb497fc

Allow navigationFallback so exclude can be set

5bc0d14

Combine catch-all wildcards into one

6634aa7

don't set rewrite when redirect is set

e5ba666

FlippingBinary added 2 commits September 26, 2022 10:02

rebase: fix merge conflicts

b41a468

write new tests

db15751

geoffrich mentioned this pull request Sep 26, 2022

allow overriding default allowedRoles #60

Open

test navigationFallback.exclude

9c3fd8e

FlippingBinary changed the title ~~allow wildcard routes~~ Enhance custom configuration support Sep 27, 2022

FlippingBinary added 4 commits September 27, 2022 11:48

Remove CustomStaticWebAppConfig

4209506

Add type hint for adapter

73267d4

Change generateConfig signature and fix bug

442ea5e

Allow customization of node version

072b5dc

geoffrich reviewed Oct 5, 2022

View reviewed changes

Update index.js

93d6908

Co-authored-by: Geoff Rich <4992896+geoffrich@users.noreply.github.com>

FlippingBinary added 2 commits October 11, 2022 10:38

chore: merge upstream

db12d11

feat: throw error on node version less than 16

06ae5bf

geoffrich mentioned this pull request Mar 22, 2023

Use route-level config to set allowedRoles #134

Open

BernhardAuer mentioned this pull request Jul 2, 2023

Dynamically determine node version #48

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance custom configuration support #66

Enhance custom configuration support #66

FlippingBinary commented Sep 23, 2022

geoffrich commented Sep 25, 2022

FlippingBinary commented Sep 26, 2022

FlippingBinary commented Sep 26, 2022

FlippingBinary commented Sep 27, 2022

FlippingBinary commented Sep 29, 2022

geoffrich left a comment •

edited

geoffrich Sep 27, 2022

FlippingBinary Oct 5, 2022

geoffrich Oct 5, 2022

FlippingBinary Oct 5, 2022

geoffrich Oct 5, 2022

geoffrich Oct 5, 2022

FlippingBinary Oct 5, 2022

geoffrich Oct 5, 2022

FlippingBinary Oct 5, 2022

FlippingBinary commented Oct 5, 2022 •

edited

		rewrite: ssrFunctionRoute,
		...customStaticWebAppConfig.navigationFallback

Enhance custom configuration support #66

Are you sure you want to change the base?

Enhance custom configuration support #66

Conversation

FlippingBinary commented Sep 23, 2022

geoffrich commented Sep 25, 2022

FlippingBinary commented Sep 26, 2022

FlippingBinary commented Sep 26, 2022

FlippingBinary commented Sep 27, 2022

FlippingBinary commented Sep 29, 2022

geoffrich left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FlippingBinary commented Oct 5, 2022 • edited

geoffrich left a comment •

edited

FlippingBinary commented Oct 5, 2022 •

edited