sst / ion

SST v3
https://sst.dev
MIT License
2.11k stars 247 forks source link

docs: SsrSite and StaticSite edge props #1034

Closed fwang closed 1 month ago

fwang commented 1 month ago

Review the doc for:

paolostyle commented 1 month ago

I suppose it was just unfortunate timing as I can see that SST is in heavy development but I did already implement the viewerRequest through arn. So technically this is a breaking change, but I'm not sure if you're using semver anyway, so no big deal here.

The issue I have with this current implementation is that if you want to modify request.uri, which, IMO, for an SPA is a must (I'll get to that part later), the default handler makes it more difficult. I would much rather just pass the entire implementation of the CF handler than using that injection pattern. If someone wants to actually inject the value, I'd say an API where you pass a function and the first argument is the default implementation (if you want) you can interpolate would be an improvement. Something like this:

const site = new sst.aws.StaticSite('Site', {
  edge: {
   // the `function handler(event) {` and `return event.request` would still be there
    viewerRequest: (defaultImpl) => `
      ${defaultImpl}
      event.request.headers["x-foo"] = "bar";
   `,
  },
});

Or you could provide another property code (like in aws.cloudfront.Function from Pulumi where you would actually replace the function code. Or just provide the default implementation in the docs, only the code attribute in viewerRequest and if they want to extend it, they'd just have to copy paste it alongside their implementation. But the current option is just weird.

As for why I think the default handler is wrong for SPAs: with plain Vite + React app with client side routing, you generally have just a single index.html and a bunch of assets. So with the current function, if a user goes to e.g. /sign-up, will redirect them to /sign-up.html, which obviously does not exists. This works currently only because the CloudFront config by default also assumes that on 404 errors a user should get redirected to index.html. If you change the error page, the entire routing stops working. Another issue with this approach is that index.html is practically uncacheable (HTML files are not cached by default in StaticSite - they could be though, considering that by default invalidations run on /*), because for any non / or /index.html route you'll be getting X-Cache: Error from CloudFront header. I'm not saying the default behavior is completely wrong, it probably works fine for SSGs where you actually have as many HTML files as there are routes on the site. But for SPAs it's far from optimal.

What I've been doing for the last couple of years is more or less this (previous iterations were using var, .indexOf etc. so it would work with cloudfront-js-1.0):

const assetExtensions = ['.js', '.css', '.svg', '.json', '.woff2', '.webp', '.png'] //...the list goes on but it's not that long
// it could also be solved by a regex that does not match `.html?`
function handler(event) {
  if (!assetExtensions.some((ext) => event.request.uri.endsWith(ext))) {
    event.request.uri = '/index.html';
  }
  return event.request;
}

Now, this particular one should still work, even after the modifications from the default implementation, but I know many people also use a condition which only checks whether . is in the request.uri and this one would not work anymore because the request.uri is modified. Generally, just the fact that you no longer have access to the original value of request.uri is strange. So I'd ask you to reconsider the current solution, I don't know which option would be the best, but personally I don't think the current one is the right one.

jayair commented 1 month ago

@paolostyle Yeah we can support it taking an ARN. Create a new issue?