Skip to content
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

RFC: Redirects/rewrites/headers support #587

Closed
dphang opened this issue Sep 7, 2020 · 24 comments
Closed

RFC: Redirects/rewrites/headers support #587

dphang opened this issue Sep 7, 2020 · 24 comments
Assignees
Labels
enhancement New feature or request

Comments

@dphang
Copy link
Collaborator

dphang commented Sep 7, 2020

Just starting this thread about custom redirects/rewrites/headers support. It seems to be the most requested feature in Next.js 9.5 as of now, so I think it makes sense to support this next.

Next.js docs:

Redirects: https://nextjs.org/docs/api-reference/next.config.js/redirects
Rewrites: https://nextjs.org/docs/api-reference/next.config.js/rewrites
Headers: https://nextjs.org/docs/api-reference/next.config.js/headers

I am grouping these three features together they are similar and were added at the same time:

  • The data for this is stored in routes-manifest.json after Next.js build, so it's already accessible in the handler code. Note, Next.js does have the regexes stored here as well.
  • They all support hardcoded routes + path matching: simple matching, wildcard matching, and regex matching.

Note that there is existing code that I wrote to handle trailing slash redirects (which is not using the default-added redirect in routes-manifest.json right now for perf reasons)

Since we are now using Rollup.js to bundle the handlers, we can write separate files for better maintainability. I suggest the following steps:

  • Create a matcher class that will do all matching including simple path matching, wildcard path matching, and regex matching. This code is shared with header/rewrite/redirects and other functionality that may need it in the future.
  • Redirects/rewrites: in origin request handler, create new functions/classes to handle rewrites/redirects. For example, for redirects, we would (1) refactor existing trailing slash redirect code into this class, and then (2) add support for custom redirects. Similar for rewrites. These would be done close to the start (e.g replacing the redirect code).
  • Headers: create new functions/class to add headers and use it in origin response handler (public files, HTML pages). I don't believe it's needed in origin request handler (SSR pages) as you can just set res headers in the page code itself.
  • In each of these classes (redirect/rewrite/headers), at a high level, we would:
  1. Load routes-manifest.json for the redirects/rewrites/headers list.
  2. Check custom paths against the matcher.
  3. Do the appropriate action: e.g return redirect response (with correct status code), rewrite the URI, or add a header.

I imagine the tricky part would be the path matching.

I'm already working on the e2e tests, so if someone wants to start on this or has any thoughts on the design, feel free to add your thoughts here. If not, I will work on this sometime later this month.

@danielcondemarin FYI.

@dphang
Copy link
Collaborator Author

dphang commented Sep 25, 2020

Update: sorry for the delay, I have been busy helping stabilize this component with end-to-end tests and other bug fixes. I will start working on the matcher next (probably this weekend or so)

@dphang
Copy link
Collaborator Author

dphang commented Sep 26, 2020

Have created the first PR (matcher + redirects): #627. @danielcondemarin could you please review it when you have time?

@dphang dphang pinned this issue Sep 27, 2020
@dphang dphang self-assigned this Sep 28, 2020
@dphang
Copy link
Collaborator Author

dphang commented Sep 29, 2020

Have published the redirect support in the latest alpha: https://www.npmjs.com/package/@sls-next/serverless-component/v/1.17.1-alpha.1. If you like, please try it out and let us know if you have any issues (Caveat: it doesn't support redirects from API routes yet, will be adding in a future PR).

@panbanda
Copy link

Thank you for doing that! I just tried it with my codebase and got InvalidArgument: The parameter MinTTL is required.

Here is my next config:

const withImages = require('next-images');

module.exports = withImages({
  async rewrites() {
    return [
      {
        source: '/sitemap.xml',
        destination: '/api/sitemap',
      },
      {
        source: '/feed',
        destination: '/api/feed?type=rss',
      },
      {
        source: '/feed.rss',
        destination: '/api/feed?type=rss',
      },
      {
        source: '/feed.atom',
        destination: '/api/feed?type=atom',
      },
    ];
  },
  async redirects() {
    return [
      // Article redirects
      {
        source: '/articles/:slug',
        destination: '/community/articles/:slug',
        permanent: true,
      },
    ];
  },
});

You mentioned no redirects for APIs but I am assuming that meant redirecting from API not to API, I could be wrong though I'll look.

@dphang
Copy link
Collaborator Author

dphang commented Sep 30, 2020

@panbanda yes right now having an /api/* path as a source of a redirect is not supported because we would need to update the API handler to do so.

For that minTTL issue, please post your serverless.yml config. Are you using custom cache behaviors? Since 1.17, you need to set minTTL, defaultTTL and maxTTL if you are setting a TTL (previously this was just ttl).

@panbanda
Copy link

panbanda commented Oct 1, 2020

Yep you were right. Looks great redirects are working well! Should we start another issue for rewrites to /api/* or doing that here as well?

@dphang
Copy link
Collaborator Author

dphang commented Oct 1, 2020

@panbanda The latest alpha should work with API paths as a redirect source too. Please try out and let us know if there are issues.

@panbanda
Copy link

panbanda commented Oct 4, 2020

Looks good but I am noticing something in the alpha3 branch. My redirects that go to the same endpoint have a query:

sitemap.xml -> /api/sitemap  <- This works great
feed -> /api/feed?type=rss  <- This is a 404.  I use atom and rss

I also tried deploying with the rewrites and neither worked. I haven't checked to see if rewrites were implemented yet.

  async rewrites() {
    return [
      {
        source: '/sitemap.xml',
        destination: '/api/sitemap',
      },
      {
        source: '/feed',
        destination: '/api/feed?type=rss',
      },
      {
        source: '/feed.rss',
        destination: '/api/feed?type=rss',
      },
      {
        source: '/feed.atom',
        destination: '/api/feed?type=atom',
      },
    ];
  },

@dphang
Copy link
Collaborator Author

dphang commented Oct 5, 2020

@panbanda yeah rewrites are not implemented yet.

Do you mind creating a new issue for the redirect bug, just so when it's fixed it's easier to reference that as its own issue.

I'll take a look later this week, but curious what does the Lambda respond with? Does it still respond with a 308 (redirect), just that the location header is wrong so it 404s?

Edit: found the problem, it's because of the query string at the end, path-to-regexp assumes it means the token before it is optional. Need to escape the querystring ? separator before compiling the destination

@dphang
Copy link
Collaborator Author

dphang commented Oct 7, 2020

The latest alpha should have the fixes for redirects, and also adds rewrites support (except for external URLs are not implemented yet): https://github.com/serverless-nextjs/serverless-next.js/releases/tag/%40sls-next%2Fserverless-component%401.18.0-alpha.2.

Please try it out and open an issue in case of any bugs.

@benjipott
Copy link
Contributor

Good job it try it today

@justingshreve
Copy link

This great, but not working for me? I'm on 1.18.0-alpha.2 and I see routes in routes-manifest.json, but not redirecting once deployed. I'm just getting 404. When I run nextjs locally, it does redirect. My next.config.js look like this:

module.exports = withSourceMaps(withImages(withCSS(withBundleAnalyzer({
  target: 'serverless',
  cssLoaderOptions: {
    url: false
  },
  env: {},
  webpack: (config, options) => {
    // some configs
  },
  async redirects() {
    return [
      {
        source: '/dashboard',
        destination: '/otherplace',
        permanent: true
      }
    ]
  }
}

Am I missing a config or setting?

@dphang
Copy link
Collaborator Author

dphang commented Oct 8, 2020

@justingshreve the handler code should parse the redirects from routes-manifest.json which should be in the Lambda package as well. Can you please share the routes-manifest.json and index.js (this is the handler code that does it) in the generated .serverless-nextjs/default-handler directory. It would help to debug what's wrong.

@tyler-ground
Copy link

Just wondering, having trouble grokking it reading this thread, did the new header functionality get into the alpha?

@dphang
Copy link
Collaborator Author

dphang commented Oct 8, 2020

@tyler-ground headers are not yet there as there is some implementation challenge due to the request.uri getting overwritten to the S3 key for static page / data requests, it's a bit tricky to know the original path that should get the custom headers. I am working with @danielcondemarin on how to simplify this, possibly by getting rid of the origin response handler and having origin request handler directly call S3 (i.e effectively bypassing the CloudFront S3 origin, though it can still be cached and surprisingly this way also had better performance for cache misses in my tests).

But we can first implement custom headers for any response returned in the origin request handler (SSR pages, SSR data requests, redirects etc).

For future reference you can check the features in https://github.com/serverless-nextjs/serverless-next.js#features or changelogs in https://github.com/serverless-nextjs/serverless-next.js/releases (I know it's a little hard to parse as there are many dependent packages whose changelogs don't get rolled up into the main package's, we are trying to see how we can make this cleaner).

@tyler-ground
Copy link

Thanks! I saw the headers ticket closed and this was talking about features in alpha-3. The readme has "Custom Headers" listed as a feature so I assumed it was probably done but it wasn't working for me. :)

@dphang
Copy link
Collaborator Author

dphang commented Oct 8, 2020

@tyler-ground yeah, it is listed but the checkbox is not ticked yet, so it is not ready. I'll update the readme to make it more clear.

@tyler-ground
Copy link

OH! haha, I completely didn't notice that. Thanks :)

@tyler-ground
Copy link

tyler-ground commented Oct 10, 2020

@justingshreve Did redirects start working for you? I'm on 1.18.0-alpha.4 but they don't work (I get a 404 page instead). However, I swear these were working in an earlier build but I've gone back and had no success. They do work locally.

Edit: I wonder if this is perhaps another case of the wrong lib getting into the builds.

@dphang
Copy link
Collaborator Author

dphang commented Oct 10, 2020

@tyler-ground, it should not be, just deployed from 1.18.0-alpha.4 and it works fine. I did fix lerna publish (back in one of the 1.17 alphas) to publish with exact versions of dependent internal packages, so this should no longer be an issue: https://github.com/serverless-nextjs/serverless-next.js/blob/master/package.json#L22-L24

To be sure, I checked 1.18.0-alpha.4's package.json and it looks good (all @sls-next versions are pinned):

  "dependencies": {
    "@serverless/aws-s3": "^4.2.0",
    "@serverless/core": "^1.1.2",
    "@sls-next/aws-cloudfront": "1.3.0-alpha.1",
    "@sls-next/aws-lambda": "1.0.3-alpha.0",
    "@sls-next/cloudfront": "1.0.4-alpha.0",
    "@sls-next/domain": "1.1.0-alpha.2",
    "@sls-next/lambda-at-edge": "1.7.0-alpha.6",
    "@sls-next/next-aws-cloudfront": "1.5.1-alpha.0",
    "@sls-next/s3-static-assets": "1.3.1-alpha.0",
    "aws-sdk": "^2.702.0",
    "fs-extra": "^9.0.1"
  },

I also checked @sls-next/lambda-at-edge@1.7.0-alpha.6 for the distributed default-handler.js and it has the redirect logic.

Are you able to share the build output files in .serverless_nextjs/default-lambda (or api-lambda)? Specifically, index.js (the handler) and routes-manifest.json? It would help understand why it is not working.

Also, do be sure that the Lambda being called has already been deployed to your closest CloudFront edge location (in case perhaps you were switching between 1.17 (which has no custom redirects) and newer 1.18 alphas, etc.) - you can verify by checking an SSR page and making sure the build version in the page source matches the one you recently deployed. Usually it's within a couple of minutes, but a few times it took a long time for me, like 15-30 minutes.

Thanks.

@dphang
Copy link
Collaborator Author

dphang commented Oct 10, 2020

Headers support has been released in the latest alpha (https://github.com/serverless-nextjs/serverless-next.js/releases/tag/%40sls-next%2Fserverless-component%401.18.0-alpha.5) - with a caveat due to our use of S3 origin / origin response handlers and rewriting the URI to the S3 key. E.g if you want to add a header to any path mapping to an S3 object, such as an SSG page, you must specify the source as the S3 key (without prefix such as public, static-pages e.g /page.html) instead of the URL path.

We will work on addressing this caveat in the future.

Please try out and let us know of any bugs by opening a new issue.

PS: a couple of people have mentioned that these new features are not working for them. I am unsure why that's the case, as I did check the latest 1.18 alpha versions and confirmed they are using pinned internal dependencies and have the redirect/rewrite/header logic in the handler code. If it's not working, I suggest the following:

  1. Ensure your Lambda handler is fully deployed to CloudFront by confirming the build version (matches the one in .serverless_nextjs/default-lambda/manifest.json or .next/BUILD_ID) on a page generated by a Lambda. It should mostly update within a couple of minutes, but in rare cases, it has taken longer for me (up to 30 minutes).
  2. Ensure that responses are not cached by CloudFront or your browser (e.g a path that was previously a 404, but you now want to redirect, is cached by CloudFront for some time, so it could still return a 404). However, all paths should be invalidated from the cache (/*) by the component. But it may take some time to complete all invalidations. You can check the status in the CloudFront console.
  3. Clear npm cache (locally or on CI), which I believe is where Serverless Components downloads any package versions - in case somehow it's trying to use an incorrect @sls-next/serverless-component version. I think this is unlikely, but maybe there is a bug in Serverless Components?
  4. Check serverless.yml and make sure you pin @sls-next/serverless-component to one of the recent/latest alpha versions (latest tag does not have redirect/rewrites/headers changes yet). Do not add @sls-next/serverless-component in package.json.
  5. If the above does not work, please open a new issue and post your routes-manifest.json (contains your redirects/rewrites/headers) and index.js (the handler code that contains redirect/rewrite/header logic) in your .serverless_nextjs/default-lambda or .serverless_nextjs/api-lambda build output folders. This is the package uploaded to Lambda@Edge.

Thanks!

@tyler-ground
Copy link

@dphang Thanks, my deployment systems are setup such that I don't retain artifacts but when I'm home later this weekend I'll modify them to store the files you requested in S3 and include. I am 100% sure as I waited 5 minutes between each deploy (I spent a few hours messing around with this). 👍

@dphang
Copy link
Collaborator Author

dphang commented Oct 30, 2020

Closing this issue for now as I believe all of the functionality is implemented, except with the caveats are mentioned in the README but it is due to how the component is architected (e.g due to having both origin request/response handlers, and some cache behaviors do not have handlers to do rewrite/redirect/headers functionality). Will revisit any improvements in the future.

@dphang dphang closed this as completed Oct 30, 2020
@dphang dphang unpinned this issue Oct 31, 2020
@benjaminkay93
Copy link

@dphang should this now work with external URL's, this is the feature we would really appreciate for our go live with next js, is there anything you would need help with doing that to get it out ASAP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants