-
Notifications
You must be signed in to change notification settings - Fork 33
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
Enhance custom configuration support #66
base: main
Are you sure you want to change the base?
Enhance custom configuration support #66
Conversation
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:
|
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:
Also, right now |
This could close #48, but I wonder if it should enforce SvelteKit's minimum node version instead of allowing any node version? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
if (['/*', '*'].includes(route.route)) { | ||
route.route = '*'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is /*
equivalent to *
? i.e., do they both match https://svelte.dev and https://svelte.dev/docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested that by setting both routes, but SWA rejected the duplicate.
if (swaConfig.navigationFallback.rewrite === ssrTrigger) { | ||
swaConfig.navigationFallback.rewrite = ssrFunctionRoute; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
rewrite: ssrFunctionRoute, | ||
...customStaticWebAppConfig.navigationFallback |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there's a good reason to allow for customization of navigationFallback.rewrite
(see other comment), these lines should be swapped
rewrite: ssrFunctionRoute, | |
...customStaticWebAppConfig.navigationFallback | |
...customStaticWebAppConfig.navigationFallback, | |
rewrite: ssrFunctionRoute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
Just to clarify: This pull request does not make it necessary to write There are two reasons why someone might need to put
In my case, I currently have a POST endpoint that has to have a This is why use case 2 in your list is important. You make a really good point about SvelteKit slipping past 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. |
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 orrewrite: undefined
to disable it entirely. It allows the custom configuration to definenavigationFallback
so theexclude
rule can be set. It also allows theplatform.apiRuntime
to be overridden but will throw an error if it is set to a string that doesn't start withnode:
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.