svelte-adapter-azure-swa
svelte-adapter-azure-swa copied to clipboard
Enhance custom configuration support
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.
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.
Absolutely! I'm glad you added unit testing. I'll merge the changes soon, probably tomorrow, and update this pull request.
@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 arewriteanywhere, 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 thegenerateConfig(...)function is entirely responsible for generating the config. Nothing else changes it. - I switched from
.toStrictEqualto.toEqualfor testing the config because the strict version tests for differences that do not carry throughJSON.stringifyso they don't matter anyway.
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
generateConfigfunction signature togenerateConfig(builder, customStaticWebAppConfig). The current version still has some code I copied fromadaptthat expectsbuilderto be there, which I didn't notice because it didn't cause a problem when it failed to run. This change puts thecustomStaticWebAppConfigin a different position than before, but it's not a public function and the changed order makes it possible to givecustomStaticWebAppConfiga default value properly. - Remove the
CustomStaticWebAppConfigtype and replace it withStaticWebAppConfigbecause 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.
This could close #48, but I wonder if it should enforce SvelteKit's minimum node version instead of allowing any node version?
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
navigationRewritebut 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.